-
Notifications
You must be signed in to change notification settings - Fork 62
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
Delete Simple IEDO GUI Flowsheets #1417
Delete Simple IEDO GUI Flowsheets #1417
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1417 +/- ##
==========================================
- Coverage 94.01% 93.81% -0.21%
==========================================
Files 335 322 -13
Lines 35570 34379 -1191
==========================================
- Hits 33442 32252 -1190
+ Misses 2128 2127 -1 ☔ View full report in Codecov by Sentry. |
@lbianchi-lbl codecov isn't making sense here either. Patch passes but project decreases by ~3%, but the PR only removes lines of code and doesn't touch any tests. |
Yeah, it looks like it might have been a transitory thing since the problem seems to have gone away on the other affected PRs after updating from |
def adjust_default_parameters(m): | ||
m.fs.metab_hydrogen.hydraulic_retention_time.fix(6) # default - 12 hours, 0.5x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimBartholomew Is it fine to delete this function. Codecov was failing because of this, and it was apparently only used in the metab_ui.py
file, which has been deleted.
@dangunter @MichaelPesce Tagging the two of you here since I think you're the most familiar with the |
@MarcusHolly As far as I can tell, the change made in |
@MarcusHolly I agree with @MichaelPesce, removing code generally shouldn't result in a significant change in coverage (assuming the code being removed had a similar coverage to the project as a whole). The detailed summary on the Codecov webpage seems to show no significant changes, so the failing check is likely to be a fluke. Let's see if updating the head branch as you've just done resolves this. |
@lbianchi-lbl It's still failing the codecov check. Is it acceptable to just bypass it (i.e. ignore the failure)? |
@MarcusHolly I'm looking into this more carefully, and it seems that one of the flowsheets, namely Since the corresponding entry point in Is there any obstacle in deleting that file altogether? If not, could you try to do that and see if that resolves the coverage check failure? |
So I've deleted the METAB UI files, but I don't think we've reached a verdict yet on whether or not we should be deleting these flowsheets just yet (we discussed in the dev call last week and it seemed like we were leaning towards keeping these flowsheets here rather than archiving them elsewhere). The only reason this METAB flowsheet was modified is because that one function was only being used in the METAB ui flowsheet (and METAB ui flowsheet test file). None of the other flowsheets have a function like this. EDIT: I could temporarily delete this file "for science" and see if the test passes, but we'd have to undo it afterwards. |
@MarcusHolly I think I understand now, thanks for the clarification. I think I mistakenly assumed that both the flowsheet and the UI code were being removed by this PR, but as you point out, that's not actually the case. In that case, I think we can ignore the Codecov check failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go if Tim agree to delete the functions for METAB analysis
@bknueven @lbianchi-lbl (I'm assuming one of you two will merge this) This PR can be merged, but we'll have to bypass the codecov check as mentioned here: #1417 (comment) |
Changes proposed in this PR:
setup.py
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: