Removed hard-coded logic for electrolyzer replacement schedule#555
Removed hard-coded logic for electrolyzer replacement schedule#555elenya-grant wants to merge 8 commits intoNatLabRockies:developfrom
Conversation
…chedule for transport components
jaredthomas68
left a comment
There was a problem hiding this comment.
This looks pretty good to me. Thanks for continuing to generalize and reduce tech-specific logic. My main thoughts are as follows:
- Let's allow for costs for transport techs (needed in current project)
- I don't love having costs or replacement schedule in feedstocks, but if the user does not need to do anything with them then I think it is ok if keeps things more general.
| if tech_name == "cable" or tech_name == "pipe": | ||
| continue |
There was a problem hiding this comment.
I think we should not limit the transporters to not having costs. Let's include a replacement schedule for them like you did for feedstocks (though I think it makes less sense for feedstocks but I get the generality). We have a current project where we will likely need to include transport costs.
There was a problem hiding this comment.
Transporters are not limited to not have costs. The "cable" and "pipe" transporters are "built-in" H2I transporters and are just pass-through components that do not have an associated cost model. Similarly, these technology names are protected so that a user cannot, for example, use these technology names with a generic transporter or any other transport model. Without this continue statement, there would be an error because these models don't have costs. If you put "cable" in the technologies for a finance subgroup (using the develop branch as-is) you'll get an error.
Transporters can have costs, an example of this is shown in Example 21 that uses the iron ore transport cost model. Theres also a generic transporter performance model (this is used in Example 21 and Example 31). You could add in a new transport cost model and include it in a the technologies list for a finance subgroup and its cost will be included in the finances.
The replacement schedule thing in transport cost models is also a bit tricky - because it'd likely have to be included in transport cost models. I will play around with it and see if it can be easily implemented, but I'm concerned that there may be an edge-case that would result in unexpected issues. I also think that since we have very few transport technology models, we may better understand the developments needed there as more are added. But - the logic wouldn't be difficult to change either way, so I'll try it out and comment on any findings.
There was a problem hiding this comment.
Okay - it's pretty easy to add the replacement_schedule as an output to the iron ore transport performance model and the generic transport performance model.
The tradeoff is that we have no transport model baseclass to enforce that replacement_schedule has to be output from transport models (not a huge deal but worth considering). This was different with feedstocks because feedstocks already have special handling and the feedstock model is unique, whereas we would expect more transport models to developed in the future. So - it makes sense to add the replacement schedule as an output to the feedstock model because it's commonly used but we don't expect more feedstock models to be added. Whereas for the transport models (which are not unique and we do expect more transport models to be added), we would need the replacement schedule to be output from all transport models and we do not have a way of enforcing that at the moment.
Aka - it could go either way. I can't think of any examples where the transport performance schedule would output a replacement schedule that isn't all zeros, but if that does seem like something we may expect, then I can make the change so that replacement_schedule is added as an output to the current transport models.
There was a problem hiding this comment.
Thank you for the detailed explanation. I wonder if we should keep the logic as is, but maybe change the names of pipe and cable to something that indicates their "pass-through" natures, such as pipe-passthrough and cable-passthrough. Given they are transporters, I do see possible confusion with those names, but I think it works as a catch-all to indicate no loss, no cost, no replacement, etc.
I'm also ok with leaving as you have it here given your above comments.
Removed hard-coded logic for electrolyzer replacement schedule
Removed the hard-coded logic within the finance models and H2IntegrateModel that was specific to connecting the replacement schedule from the technology to a finance model if a technology name included "electrolyzer". Now - the replacement schedule is passed to the finance models for all technologies.
Note: This is ready for an in-depth review, but I have noted some specific questions for reviewers in Section 2 under "Type of Reviewer Feedback Requested"
Since technology-specific finance models do not expect the electrolyzer replacement schedule to be connected, there is some added logic to handle tech-specific finance models (before, the logic was not explicit and relied on whether the
commodity_streamfor a finance subgroup was None. This logic has been replaced with more explicit logic. Additionally, since this logic is handled/defined at the subgroup-level, it is not possible to have a finance subgroup that includes finance_groups that have a mix of technology-specific and finance specific models (this has always been the case). Further information on this is described in Section 5. But - I added in an error message to catch this case if it were to pop up.This also required updating the feedstock cost model to output a replacement schedule. An alternative to adding this output to the feedstock cost model would be to add in logic to
connect_technologiesthat checks if 'feedstock' is in the technology name (this alternative approach was the route taken for transport technologies).I also added a documentation page about the technology naming convention in H2I to communicate what naming convention is requires so that the logic within H2I functions as expected.
Section 1: Type of Contribution
Section 2: Draft PR Checklist
TODO
Type of Reviewer Feedback Requested (on Draft PR)
Structural feedback:
Implementation feedback:
replacement_scheduleor should we introduce logic intoconnect_technologiesso that technologies withfeedstockin the name do not have the replacement schedule passed to the finance model? This logic would maybe have to be duplicated in the finance models if so. I like the current implementation since it reduces the amount of naming-specific logic in the code base.Other feedback:
default_techs_to_commoditiesdictionary inH2IntegrateModel.create_finance_model()) be described in thedocs/user_guide/expected_naming_convention.mddoc page?Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.mdhas been updated to describe the changes made in this PRSection 3: Related Issues
This is somewhat related to Issue #485
Section 4: Impacted Areas of the Software
Section 4.1: New Files
docs/user_guide/expected_naming_convention.md: new doc page that explains what naming convention is needed for technologies in H2I so that it functions properlySection 4.2: Modified Files
h2integrate/finances/profast_base.py-ProFastBase: removed logic around electrolyzer replacement schedule, added input for all replacement schedule for all technologies, and updated replacement schedule calculation to useinputs['replacement_schedule_{tech}']instead of time between replacement.examples/08_wind_electrolyzer/user_finance_model/simple_lco.py: added input of technology replacement schedulesh2integrate/finances/numpy_financial_npv.py-NumpyFinancialNPV: removed logic around electrolyzer replacement schedule, added input for all replacement schedule for all technologies, and updated replacement schedule calculation to useinputs['replacement_schedule_{tech}']instead of time between replacement.h2integrate/core/feedstocks.py: addedreplacement_scheduleoutput toFeedstockCostModelh2integrate/core/h2integrate_model.py:create_finance_model():system_finance_modelkey to thefinance_subgroupsdictionary. This is a boolean that is True if the finance model is system-level and False if the finance model is tech-specific.n_tech_finances_in_groupthat is equal to the number of technology-specific finance models within a finance subgroup. This is used so that later on, we can throw an error message if someone has a mix of technology-specific finance models and system-level finance models within one finance subgroup.connect_technologies()elifstatement forelif "storage" in source_techandelif "storage" in dest_techbecause it was the same logic that is done in theelsestatement and was therefore unnecessary.if commodity_stream is not Nonetoif is_system_finance_modelwhich is pulled from thefinance_subgroupsattribute that is created increate_finance_model()is_system_finance_modelis True andtech_namedoes not include "transport".h2integrate/core/test/test_framework.py: added new test,test_invalid_finance_group_combination, for error message addedSection 5: Additional Supporting Information
Suppose we have a technology-specific finance model for the
steeltechnology and a system-level finance model namedprofast_model(as is the case in Example 1). If a user tried to run both of these finance models as shown below:there would be an error (this PR now includes a more verbose error than the error that would've occurred when trying to run H2I).
The user should instead define two finance subgroups, one per finance model.
This error does not come up if finance models defined in the
finance_groupsfor a specific finance subgroup are either all tech-specific finance models OR all system-level finance models (see example 8).Section 6: Test Results, if applicable