Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjustments regarding the new solph structure #21

Merged
merged 13 commits into from
Mar 29, 2022

Conversation

fwitte
Copy link
Member

@fwitte fwitte commented Apr 7, 2020

No description provided.

@simnh
Copy link
Member

simnh commented Jun 9, 2020

@eosram @gnn can you please check ?

@gnn
Copy link
Member

gnn commented Jul 15, 2020

Sorry for taking so long. I'm not ready to approve this yet for two reasons:

  • I still have to get oemof.network out of release candidate status, which entails moving a lot of classes back into the oemof.network module, which means a lot of the import adjustments can be rolled back.
  • I still have questions about some of the adjustments.

@simnh
Copy link
Member

simnh commented Jul 16, 2020

Ah yeah there is also still a problem thats maybe linked to the network module:

/home/admin/projects/oemof-jordan/jordan-env/lib/python3.6/site-packages/oemof/solph/network.py:263: SuspiciousUsageWarning: `Source` 'Aqaba1' constructed without `outputs`.

This similar warning actually happens for every components being instantiated in the one way or another.

@uvchik
Copy link
Member

uvchik commented Jul 16, 2020

You should switch off the SuspiciousUsageWarning. This was a compromise between solph and tabular. For most solph users it is an error if they construct a Source without an output or a Sink without an input. If the user just misspell the respective attribute it was hard to find the bug. So we invented the SuspiciousUsageWarning to make it easier for them to find such bugs.

But it is allowed to construct a Source without an output and add the output later. In solph this is very rare but it is the way you construct nodes in tabular. Therefore, we do not raise an error and we added a specific warning that do not effect other warnings if you switch it off (see Handling of Warnings in the solph docs).

@unndreay
Copy link

Hey! We are using tabular in https://github.com/modex-flexmex/oemo-flex/ and wait for it to get working with oemof-solph 0.4.x. Since I am not familiar with the background, I couldn't see what's the progress or what solution is needed. Can we help at some point or take action ourselves? Thank you!

@simnh
Copy link
Member

simnh commented Jan 16, 2021

@uvchik Thanks for the hint with the warning!

@unndreay Yeah acutually most of the work has been done. The warning stuff that @uvchik mentioned has to be done. And I guess some final checks. You could help be dealing with the warning

Also @gnn wanted to transfer some code from anther dependency to this package which is kind of important. So it would be great if you @gnn would finalize this PR (which is pending for quite a while....)

I used the version for some projects already, and I am sure everthing works as intended.

@simnh simnh added the help wanted Extra attention is needed label Jan 16, 2021
@simnh
Copy link
Member

simnh commented Jan 16, 2021

Hey! We are using tabular in https://github.com/modex-flexmex/oemo-flex/ and wait for it to get working with oemof-solph 0.4.x. Since I am not familiar with the background, I couldn't see what's the progress or what solution is needed. Can we help at some point or take action ourselves? Thank you!

dfg

@simnh
Copy link
Member

simnh commented Jan 16, 2021

Hey! We are using tabular in https://github.com/modex-flexmex/oemo-flex/ and wait for it to get working with oemof-solph 0.4.x. Since I am not familiar with the background, I couldn't see what's the progress or what solution is needed. Can we help at some point or take action ourselves? Thank you!

You could try to fix the stuff that causes the checks to fail :-) (if that possible for you)

@jnnr
Copy link
Member

jnnr commented Jan 18, 2021

@uvchik Thanks for the hint with the warning!

@unndreay Yeah acutually most of the work has been done. The warning stuff that @uvchik mentioned has to be done. And I guess some final checks. You could help be dealing with the warning

It sounds like we should switch off the SuspiciousUsageWarning for oemof.tabular. This should not be too much work - probably somewhere centrally?

Also @gnn wanted to transfer some code from anther dependency to this package which is kind of important. So it would be great if you @gnn would finalize this PR (which is pending for quite a while....)

Can this be done in a separate PR?

I used the version for some projects already, and I am sure everthing works as intended.

So this branch is already tested in practice. Great!

@jnnr
Copy link
Member

jnnr commented Feb 1, 2021

With 1922cd9, I switched off the SuspiciousUsageWarnings in the facades module. If you think that it should be switched off at a different place, please correct. I tested it with the example compute.py and could get rid of the warnings.

@jnnr
Copy link
Member

jnnr commented Feb 1, 2021

We can now test this branch with our model runs for any errors. Will give feedback here. Only remaining thing is the travis check.

@jnnr
Copy link
Member

jnnr commented Feb 1, 2021

While reviewing, I noticed that commit cb1ce4d (introducing capacity_minimum) seems a bit unrelated to this PR. Wrong branch?

@simnh
Copy link
Member

simnh commented Feb 1, 2021

While reviewing, I noticed that commit cb1ce4d (introducing capacity_minimum) seems a bit unrelated to this PR. Wrong branch?

Yeah, should have been on dev...mh. maybe we can keep it here for now? I do not have time to clean this up. But I would really like to get the next version of oemof-tabular out there, which is working on 0.4 solph

@simnh
Copy link
Member

simnh commented Feb 1, 2021

@gnn can you check this branch as well? so we can continue on dev....and release...should also be important for open-modex I guess?

@jnnr
Copy link
Member

jnnr commented Feb 1, 2021

While reviewing, I noticed that commit cb1ce4d (introducing capacity_minimum) seems a bit unrelated to this PR. Wrong branch?

Yeah, should have been on dev...mh. maybe we can keep it here for now? I do not have time to clean this up. But I would really like to get the next version of oemof-tabular out there, which is working on 0.4 solph

Directly on dev 😱 ? I remember a meme for that :) ... Anyway, I am ok to keep it here if it would eventually end up in dev anyway.

Just some human and machine checks to pass - hopefully we can finish this soon!

@jnnr
Copy link
Member

jnnr commented Mar 1, 2021

Any news here? Only the failing checks are still open. I found an import error in one example (fixed in 0609ea0).

I ran tox locally. There seem to be still a problem with codecov. And an error with the jupyter notebook environments. Can someone help?

@simnh
Copy link
Member

simnh commented Mar 1, 2021

@gnn could you check thiss and we finalise this PR?

gnn added 3 commits March 1, 2021 22:21
One line was beyond 79 characters, a hard PEP 8 limit for everything.
This prompted me to also look at other lines which where longer than 72
characters, a PEP 8 limit for comments and other free flowing text. I
reformatted the lines to stay within the character limit.
While at it, I also changed the first line of the docstring to be a
the regular sentence that it is.
Write imports from the same module on one line to squelch `isort` errors.
@gnn
Copy link
Member

gnn commented Mar 1, 2021

On it. Sorry for being a blocker on this for far too long. I fixed the tox -e check fails. Don't know whether I can fix the ipython stuff or the other build errors, but I'll have a look at it and request assistance if I can't fix the errors myself.

simnh added 2 commits May 12, 2021 09:25
This is a requirement for v0.4 solph, as fuchur is not maintained and 
does not work with v0.4. But fuchur is required to use oemof tabular 
with datapackages when constructing those
@jnnr
Copy link
Member

jnnr commented Oct 21, 2021

Any news here? Would be a huge help if we could finish this soon...

@MaGering
Copy link
Contributor

I also wanted to check if this PR could be merged soon? Tests fail on my branch rl-institut/oemof-B3#76 because of an incompatibility error of pandas caused by oemof.tabular's installation of oemof (cf. this comment).

@jnnr
Copy link
Member

jnnr commented Mar 3, 2022

Tested this branch with some datapackages and encountered the following problem: When building instances of the tabular facade Link, an AssertionError is raised in the init of oemof.solph Link https://github.com/oemof/oemof-solph/blob/dev/src/oemof/solph/custom/link.py#L88-#L90. This happens because the inputs, outputs and conversion factors are not set in the facades init_, but later, when build_solph_components is called. Any ideas how to handle this? Update: Opened a PR in oemof.solph that aims to fix it.

@henhuy henhuy merged commit 3d3a3bc into dev Mar 29, 2022
@henhuy henhuy deleted the fix/imports_and_requirements_for_oemof_solph_040 branch April 19, 2022 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants