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

Replace constraint tests #1080

Merged
merged 63 commits into from
Aug 20, 2024
Merged

Replace constraint tests #1080

merged 63 commits into from
Aug 20, 2024

Conversation

p-snft
Copy link
Member

@p-snft p-snft commented Jun 11, 2024

Replaces lp diff constraint tests by granular unit tests for functionality.

Reasoning:

  • The LP files depend on Pyomo internals, thus they change when Pyomo changes things.
  • They include tests for stuff from multiple parts of the code. For example, if I change the constraint formulation for the Bus, every constraint test needs to be rewritten, not only the one for the Bus.
  • They are hard to understand (for many) and failed tests are hard to fix. The latter is particularly because of the undefined order of lines, so that just looking at a diff will not tell the difference: Constraints might be in a different order but still the same.

I would suggest to have unit tests that

  • will be using Pyomo but the results will be independent from the back-end,
  • is easy to understand, and
  • even internal constraint formulations can be changed without breaking the test, so it really tests for the specified functionally not the implementation.

Adjust indexing to be same as for content of storage without investment.
@p-snft p-snft linked an issue Jun 11, 2024 that may be closed by this pull request
@p-snft p-snft added this to the v0.5.x milestone Jun 20, 2024
p-snft and others added 24 commits July 2, 2024 15:43
For Python 3.10+, "|" should be used instead of "or", but or is also
incorrect for Python 3.9 (a typing.Union can be used). However, as date
is superclass of datetime, giving the superclass shuld imply that the
childs are also possible.
BaseModel was an unused superclass just for Model. At the moment it is not
planned to have other models. Thus, it makes sense to merge both.

Signed-off-by: Patrik Schönfeldt <[email protected]>
It seems to be a desired (and tested) feature to do so, but a typical user
will probably not want to do this. So, a warning makes sense.
The function "test_optimal_solution" didn't actually test anything,
so I deleted it. The function "test_infeasible_model" now only tests
for the infeasibility warning.
Argument "energysystem" to Model should not be a list anymore.
The multi-period version is not tested, as both versions
will probably be unified. (And multi-period is experimental anyway.)
@p-snft
Copy link
Member Author

p-snft commented Aug 14, 2024

As I did not want to write a test assuring wrong results, I merged fix/invest_storage_content_revised. So, this one now also fixes #1030.

@p-snft
Copy link
Member Author

p-snft commented Aug 14, 2024

The remaining missing test coverage of the _generic_storage.py is related to multi-period code which is mostly duplicated code from single-period models, but also partly broken (see 1597318).

@p-snft
Copy link
Member Author

p-snft commented Aug 15, 2024

Codacy complains because I have slightly changed the signature of a complex function: It says that I introduced a complex function. (In fact, I slightly reduced the complexity by the change.)

@p-snft p-snft requested a review from jokochems August 15, 2024 19:11
@p-snft p-snft marked this pull request as ready for review August 16, 2024 08:46
@jokochems
Copy link
Member

Can I delete the automatic discounting in

else:
for i, o in self.ACTIVITYCOSTFLOWS:
if m.flows[i, o].nonconvex.activity_costs[0] is not None:
activity_costs += sum(
self.status[i, o, t]
* m.flows[i, o].nonconvex.activity_costs[t]
* m.objective_weighting[t]
* ((1 + m.discount_rate) ** -m.es.periods_years[p])
for p, t in m.TIMEINDEX
)

and simplify the code this way? The activity_costs are a time series anyway, so discounting could easily be considered externally. (@jokochems)

PS: Of course, the same is also valid for inactivity_costs, startup_costs, and shutdown_costs.

I agree that it makes sense to exclude discounting. In general, this should be harmonized across all cost types, but I think since this is a rather rare, probably unused use case here, I think it is safe to remove it here.

Copy link
Member

@jokochems jokochems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @p-snft,

thanks for this pull request and the very good effort that I highly appreciate. I left a lot of comments, but most of them are rather editorial remarks and/or questions of understanding that should be easy to address. I also like the new test framework and the examples you created, esp. concerning the non convex flow constraints that were rather untested before (though officially covered). Thus, I am happy to approve the pull request.

Nonetheless, I'd like to raise two points. I don't see them as critical, but rather want to raise awareness:

  1. It seems to me that not yet all of the constraints are tested one by one as they actually should. I didn't find an explicit test for bus balance constraints or the converter efficiencies for instance. These were at least implicity contained in the tests before, because at least I also checked these when comparing the LP files.
  2. Some experimental components did have testing before, but now they don't have it anymore.

I think it is legitimate to proceed since I do not want to interfere with your development agenda, but rather raise awareness for this issues. I think both aspects can be covered in dedicated pull requests.

Once again, many thanks for the great effort. Besides vastly improving maintainability, in my view also a further improvement in code quality.

"""
if number is None:
if calendar.isleap(year):
hoy = 8784
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it has just been moved, I suggest to rename to the more speaking hours_of_year instead of the abbreviation hoy.

src/oemof/solph/_models.py Show resolved Hide resolved
src/oemof/solph/_models.py Outdated Show resolved Hide resolved
src/oemof/solph/_models.py Show resolved Hide resolved

if reference_bus in oc_outputs:
f = oc_outputs[reference_bus]
get_slope_and_offset = slope_offset_from_nonconvex_output
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand this function reference in the first place. Is there any counter argument against renaming the function in the _offset_converter module where it is imported from?

flow = solph.flows.Flow(
nominal_value=10,
min=0.1,
max=[i * 0.1 for i in range(10)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, the range object should be redefined to start with 0.1. A max value below a min value is generally indicative for a modelling error in my view.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enforces the flow to be inactive. But you are right, this is not what we wanted. I'd suggest to have an input validation that minimum <= maximum.

Comment on lines 75 to 80
solph.constraints.generic_integral_limit(
model,
"my_factor",
limit_name="limit_my_factor",
limit=low_emission_flow_limit,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no equivalent thing for the high_emission_flow_limit.

nominal_value=2,
min=0.5,
nonconvex=solph.NonConvex(),
custom_attributes={"keyword1": "also group 1"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also like the sense of humor, but suggest to use "group 1" as the value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make the point that the value is not relevant, here.

@p-snft p-snft mentioned this pull request Aug 19, 2024
@p-snft p-snft merged commit 5706ca2 into dev Aug 20, 2024
12 of 13 checks passed
@p-snft p-snft deleted the revision/replace-constraint-tests branch August 20, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace constraint tests
2 participants