Remove more unused imports #995

wigging · 2023-03-31T18:12:31Z

Fixes/Resolves:

Resolves issue #994.

Summary/Motivation:

Remove unused imports throughout the watertap package. After this pull request is merged, the GitHub Actions should be updated to check future pull requests for unused imports.

Changes proposed in this PR:

Remove unused imports

Legal Acknowledgement

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

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

wigging · 2023-03-31T19:02:58Z

The CONFIG_Template that is imported in watertap/core/membrane_channel0d.py is unused in that file. However, if CONFIG_Template is removed then it breaks everything in watertap. It appears to be imported in the membrane_channel0d.py and then imported from that file into other files. CONFIG_Template is created in the membrane_channel_base.py so why isn't it imported from that file throughout the package?

codecov · 2023-03-31T19:17:35Z

Codecov Report

Merging #995 (0dd21fc) into main (1334584) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0dd21fc differs from pull request most recent head 54b7cde. Consider uploading reports for the commit 54b7cde to get more accurate results

@@            Coverage Diff             @@
##             main     #995      +/-   ##
==========================================
- Coverage   95.83%   95.82%   -0.01%     
==========================================
  Files         301      301              
  Lines       28801    28767      -34     
==========================================
- Hits        27601    27567      -34     
  Misses       1200     1200

Impacted Files	Coverage Δ
..._anaerobic_digester/GLSD_anaerobic_digestion_ui.py	`100.00% <ø> (ø)`
...tewater_resource_recovery/amo_1575_hrcs/hrcs_ui.py	`100.00% <ø> (ø)`
...r_resource_recovery/amo_1575_magprex/magprex_ui.py	`100.00% <ø> (ø)`
...o_1595_photothermal_membrane_candoP/amo_1595_ui.py	`100.00% <ø> (ø)`
...stewater_resource_recovery/amo_1690/amo_1690_ui.py	`100.00% <ø> (ø)`
...iomembrane_filtration/biomembrane_filtration_ui.py	`100.00% <ø> (ø)`
...e_recovery/dye_desalination/dye_desalination_ui.py	`100.00% <ø> (ø)`
...ent_removal/electrochemical_nutrient_removal_ui.py	`100.00% <ø> (ø)`
.../groundwater_treatment/groundwater_treatment_ui.py	`100.00% <ø> (ø)`
...ies/wastewater_resource_recovery/metab/metab_ui.py	`100.00% <ø> (ø)`
... and 30 more

wigging · 2023-04-04T14:02:19Z

The ConcentrationPolarizationType that is imported in watertap/unit_models/reverse_osmosis_0D.py is not used in that file. However, ConcentrationPolarizationType is imported from watertap/unit_models/reverse_osmosis_0D.py by other files. Why is this not imported from watertap/core/membrane_channel_base.py where it is originally defined?

adam-a-a · 2023-04-13T18:14:40Z

The ConcentrationPolarizationType that is imported in watertap/unit_models/reverse_osmosis_0D.py is not used in that file. However, ConcentrationPolarizationType is imported from watertap/unit_models/reverse_osmosis_0D.py by other files. Why is this not imported from watertap/core/membrane_channel_base.py where it is originally defined?

@wigging This was probably an oversight from when the RO 0D and 1D models were refactored with the new creation of the membrane_channel control volumes. I think you can alter the import to take straight from membrane_channel_base

bknueven · 2023-04-13T18:48:38Z

The ConcentrationPolarizationType that is imported in watertap/unit_models/reverse_osmosis_0D.py is not used in that file. However, ConcentrationPolarizationType is imported from watertap/unit_models/reverse_osmosis_0D.py by other files. Why is this not imported from watertap/core/membrane_channel_base.py where it is originally defined?

@wigging This was probably an oversight from when the RO 0D and 1D models were refactored with the new creation of the membrane_channel control volumes. I think you can alter the import to take straight from membrane_channel_base

I would not be in favor of doing this -- I think the correct API has these things imported from the RO module. Users using the RO module don't need to know about watertap.core.membrane_channel_base.

wigging · 2023-04-13T19:37:14Z

@bknueven The ConcentrationPolarizationType is imported from watertap.core not from watertap.core.membrane_channel_base. It is defined in watertap.core.membrane_channel_base but it is imported from core because of how it's handled in __init__.py.

adam-a-a · 2023-04-13T19:42:22Z

Hmmm.. I see @bknueven 's point though. So in that case, it wasn't an oversight. For the user's sake, it would make more sense for a user to import ConcentrationPolarizationType from the RO unit (or at least have the option to).

bknueven · 2023-04-13T19:48:13Z

Hmmm.. I see @bknueven 's point though. So in that case, it wasn't an oversight. For the user's sake, it would make more sense for a user to import ConcentrationPolarizationType from the RO unit (or at least have the option to).

Yes, the current behavior is intentional.

All the "option types" for the RO unit should be imported from a single location, IMHO.

wigging · 2023-04-14T18:58:37Z

@bknueven The enums ConcentrationPolarizationType, MassTransferCoefficient, PressureChangeType, and FrictionFactor are setup in watertap so they can be imported from watertap.core. It looks like this follows the IDAES approach where things like MaterialBalanceType and FlowDirection are imported from idaes.core.

bknueven · 2023-04-20T20:42:47Z

Let's do the following

Move watertap/unit_models/reverse_osmosis_*.py to new directory watertap/unit_models/reverse_osmosis/
Put the enums for reverse osmosis into watertap/unit_models/reverse_osmosis/__init__.py along side the unit model classes.

Then the API will be reasonably consistent between our various unit models. I would also be happy to leave the unused imports alone in these modules -- they do serve a specific purpose.

wigging · 2023-04-21T14:48:00Z

@bknueven I reverted the last two commits since the purpose of this pull request is to fix unused imports (not the API). To appease the linter and keep the original API, I added a few lines to the reverse osmosis unit models. See the latest commit. The use of the enum types and where they are located in the package should be handled better in watertap but I don't want to tackle that in this pull request.

adam-a-a

LGTM

…ery examples

…TransferCoefficient

wigging · 2023-05-12T13:26:20Z

@lbianchi-lbl Is anything else needed for this pull request?

lbianchi-lbl · 2023-05-12T13:37:09Z

@lbianchi-lbl Is anything else needed for this pull request?

Nope, thanks for bringing this over the finish line!

* Remove unused imports from core subdirectories * Remove unused imports from core * Undo CONFIG_Template removal * Remove unused imports from edb and property_models * Remove unused imports in tools * Remove unused imports in top-level of unit_models * Re-add the ConcentrationPolarizationType * Remove more unused imports in unit_models * Fix CONFIG_Template imports * Use Base_CONFIG_Template in membrane_channel0d * Remove unused imports in tests for chemistry examples * Remove unused imports in custom_model_demo examples * Remove unused imports from edb examples * Remove imports from flowsheets case_studies wastewater_resource_recovery examples * Remove unused imports from wastewastewater_resource_recovery examples * Remove some imports from electrodialysis examples * Remove imports from ion_exchange examples * Remove imports from oaro examples * Remove imports from RO_with_energy_recovery examples * Remove unused imports from full_treatment_train examples * Define build_components as variable in flowsheet_two_stage * Fix unused import warnings for ConcentrationPolarizationType and MassTransferCoefficient * Add instruction to ignore unused import warning * Enable unused-import Pylint check * Remove some more unused imports --------- Co-authored-by: Ludovico Bianchi <[email protected]>

wigging force-pushed the unused-imports branch from e7236b8 to f4586a4 Compare April 5, 2023 14:57

lbianchi-lbl assigned wigging Apr 6, 2023

adam-a-a added the software admin Issues related to requirements, releases, etc. label Apr 6, 2023

lbianchi-lbl added the Priority:Normal Normal Priority Issue or PR label Apr 6, 2023

wigging force-pushed the unused-imports branch from f4586a4 to babef5b Compare April 11, 2023 13:32

wigging force-pushed the unused-imports branch 2 times, most recently from a6b0709 to 29e849e Compare April 14, 2023 15:36

wigging marked this pull request as ready for review April 14, 2023 16:44

wigging force-pushed the unused-imports branch from 29e849e to 2a22fa2 Compare April 21, 2023 14:20

wigging force-pushed the unused-imports branch 2 times, most recently from 1b9d8db to 212059f Compare April 27, 2023 19:19

ksbeattie mentioned this pull request Apr 27, 2023

Enable additional Pylint static checks #914

Open

wigging force-pushed the unused-imports branch from 6500ab4 to d0b6bbd Compare April 27, 2023 21:07

adam-a-a approved these changes May 4, 2023

View reviewed changes

wigging force-pushed the unused-imports branch from d20208f to 9d72f48 Compare May 5, 2023 17:30

wigging added 4 commits May 11, 2023 21:01

Remove unused imports from core subdirectories

090ea3f

Remove unused imports from core

6df85c9

Undo CONFIG_Template removal

820bb32

Remove unused imports from edb and property_models

bc66d1c

wigging and others added 18 commits May 11, 2023 21:01

Re-add the ConcentrationPolarizationType

f11b978

Remove more unused imports in unit_models

9e29bd0

Fix CONFIG_Template imports

71a3afd

Use Base_CONFIG_Template in membrane_channel0d

ae7c06f

Remove unused imports in tests for chemistry examples

e021262

Remove unused imports in custom_model_demo examples

be6b87e

Remove unused imports from edb examples

56fcbf1

Remove imports from flowsheets case_studies wastewater_resource_recov…

f893236

…ery examples

Remove unused imports from wastewastewater_resource_recovery examples

c5d179c

Remove some imports from electrodialysis examples

31109d2

Remove imports from ion_exchange examples

8665efb

Remove imports from oaro examples

f7f0be7

Remove imports from RO_with_energy_recovery examples

0056fb1

Remove unused imports from full_treatment_train examples

5a3ac2b

Define build_components as variable in flowsheet_two_stage

1caed32

Fix unused import warnings for ConcentrationPolarizationType and Mass…

6b2290b

…TransferCoefficient

Add instruction to ignore unused import warning

1c3ea5b

Enable unused-import Pylint check

db56a46

wigging force-pushed the unused-imports branch from 9e82ba8 to 39ce742 Compare May 12, 2023 01:05

Remove some more unused imports

54b7cde

wigging force-pushed the unused-imports branch from 0dd21fc to 54b7cde Compare May 12, 2023 13:00

lbianchi-lbl linked an issue May 12, 2023 that may be closed by this pull request

Remove more unused imports #994

Closed

lbianchi-lbl self-requested a review May 12, 2023 13:37

lbianchi-lbl approved these changes May 12, 2023

View reviewed changes

lbianchi-lbl enabled auto-merge (squash) May 12, 2023 13:37

lbianchi-lbl merged commit a607709 into main May 12, 2023

lbianchi-lbl deleted the unused-imports branch May 12, 2023 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove more unused imports #995

Remove more unused imports #995

wigging commented Mar 31, 2023

wigging commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

wigging commented Apr 4, 2023

adam-a-a commented Apr 13, 2023 •

edited

Loading

bknueven commented Apr 13, 2023

wigging commented Apr 13, 2023

adam-a-a commented Apr 13, 2023

bknueven commented Apr 13, 2023

wigging commented Apr 14, 2023

bknueven commented Apr 20, 2023

wigging commented Apr 21, 2023

adam-a-a left a comment

wigging commented May 12, 2023

lbianchi-lbl commented May 12, 2023

Remove more unused imports #995

Remove more unused imports #995

Conversation

wigging commented Mar 31, 2023

Fixes/Resolves:

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

wigging commented Mar 31, 2023 • edited Loading

codecov bot commented Mar 31, 2023 • edited Loading

Codecov Report

wigging commented Apr 4, 2023

adam-a-a commented Apr 13, 2023 • edited Loading

bknueven commented Apr 13, 2023

wigging commented Apr 13, 2023

adam-a-a commented Apr 13, 2023

bknueven commented Apr 13, 2023

wigging commented Apr 14, 2023

bknueven commented Apr 20, 2023

wigging commented Apr 21, 2023

adam-a-a left a comment

Choose a reason for hiding this comment

wigging commented May 12, 2023

lbianchi-lbl commented May 12, 2023

wigging commented Mar 31, 2023 •

edited

Loading

codecov bot commented Mar 31, 2023 •

edited

Loading

adam-a-a commented Apr 13, 2023 •

edited

Loading