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

Delete Simple IEDO GUI Flowsheets #1417

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

MarcusHolly
Copy link
Contributor

@MarcusHolly MarcusHolly commented May 30, 2024

Changes proposed in this PR:

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@MarcusHolly MarcusHolly self-assigned this May 30, 2024
@MarcusHolly MarcusHolly added the Priority:Normal Normal Priority Issue or PR label May 30, 2024
@MarcusHolly
Copy link
Contributor Author

Looks much cleaner now:
image

@MarcusHolly MarcusHolly marked this pull request as ready for review May 30, 2024 21:09
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.81%. Comparing base (4f54f18) to head (2ccda96).

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.
📢 Have feedback on the report? Share it here.

@MarcusHolly MarcusHolly mentioned this pull request May 31, 2024
10 tasks
@TimBartholomew
Copy link
Contributor

@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.

@lbianchi-lbl
Copy link
Contributor

@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 main.

Comment on lines -483 to -484
def adjust_default_parameters(m):
m.fs.metab_hydrogen.hydraulic_retention_time.fix(6) # default - 12 hours, 0.5x
Copy link
Contributor Author

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.

@MarcusHolly
Copy link
Contributor Author

@dangunter @MichaelPesce Tagging the two of you here since I think you're the most familiar with the fsapi.py. Do you know why code coverage is decreasing in this PR? It just deletes some GUI flowsheets and I had to touch one check in test_fsapi.py since it was referencing a now-deleted flowsheet. I'm not sure if this is one of the scenarios where we should override the loss in coverage or if we need to add more checks in test_fsapi.py

@MichaelPesce
Copy link
Contributor

@MarcusHolly As far as I can tell, the change made in test_fsapi.py shouldn't decrease coverage, as it shouldn't matter which flowsheet export is used for that test. I remember there was discussion of some weird behavior with codecov last week - not sure if that's been resolved or not

@lbianchi-lbl
Copy link
Contributor

@dangunter @MichaelPesce Tagging the two of you here since I think you're the most familiar with the fsapi.py. Do you know why code coverage is decreasing in this PR? It just deletes some GUI flowsheets and I had to touch one check in test_fsapi.py since it was referencing a now-deleted flowsheet. I'm not sure if this is one of the scenarios where we should override the loss in coverage or if we need to add more checks in test_fsapi.py

@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.

@MarcusHolly
Copy link
Contributor Author

@lbianchi-lbl It's still failing the codecov check. Is it acceptable to just bypass it (i.e. ignore the failure)?

@lbianchi-lbl
Copy link
Contributor

@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 metab, shows up as modified rather than deleted:

image

Since the corresponding entry point in setup.py has been deleted, this might explain the discrepancy in coverage (there are still executable code lines left, but they are not run by any test since the UI interface has been removed).

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?

@MarcusHolly
Copy link
Contributor Author

MarcusHolly commented Jun 5, 2024

@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 metab, shows up as modified rather than deleted:

image

Since the corresponding entry point in setup.py has been deleted, this might explain the discrepancy in coverage (there are still executable code lines left, but they are not run by any test since the UI interface has been removed).

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.

@lbianchi-lbl
Copy link
Contributor

@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.

Copy link
Contributor

@luohezhiming luohezhiming left a 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

@MarcusHolly
Copy link
Contributor Author

@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)

@bknueven bknueven enabled auto-merge (squash) June 10, 2024 14:37
@bknueven bknueven merged commit ca5c6ec into watertap-org:main Jun 10, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants