Skip to content

Conversation

aspeake
Copy link
Collaborator

@aspeake aspeake commented Mar 10, 2025

Fixes #438

Improve encapsulation of ecm_prep.py by breaking out code into other modules. Only the main() function in ecm_prep.py exists outside of a class now. Below is a summary of changes.

  • New scout/utils.py module
    • with new class JsonIO containing json file handling methods previously in ecm_prep.Utils
    • with class MyEncoder that was previously stored in ecm_prep.py
    • with new class PrintFormat containing output format methods previously stored in ecm_prep.py (eg, verboseprint())
      • share the method PrintFormat.verboseprint in ecm_prep.py and in run.py
  • New scout/ecm_prep_vars.py module
    • with class UsefulVars previously in ecm_prep.py
    • with class UsefulInputFiles previously in ecm_prep.py
  • New class ECMUtils class in ecm_prep.py to store static methods
  • New class ECMPrep class in ecm_prep.py to store static methods for altering Measure and MeasurePackage instances
    • contains prepare_measures(), prepare_packages(), and split_clean_data()
  • Remove unused EPlusGlobals class and associated tests
  • Move breakout_mseg() to an instance method of the Measure class

To-do in a future PR

  • Break up ecm_prep.main(), limiting this function to just managing the ecm_prep workflow
  • New subpackage scout/ecm_prep/ which contains:
    • ecm_prep_vars.py
    • ecm_prep_args.py
    • ecm_prep_utils.py - NEW module containing ecm_prep.Utils
    • ecm_prep_measure.py - NEW module containing class Measure
    • ecm_prep_package.py - NEW module containing class Package
    • ecm_prep.py - the entry point for preparing ECMs, contains main(), prepare_measures, prepare_packages and other workflow-related functions
  • Since ecm_prep.py is a little more buried, it would be nice to add an entry point scout ecm_prep that calls scout/ecm_prep/ecm_prep.py`

@aspeake aspeake added this to the v1.2.0 milestone Mar 10, 2025
@aspeake aspeake self-assigned this Mar 10, 2025
@aspeake aspeake marked this pull request as ready for review March 11, 2025 21:53
@aspeake aspeake force-pushed the ecm_prep_reorg branch 2 times, most recently from d12e4c2 to d217f1e Compare March 18, 2025 23:42
@aspeake aspeake force-pushed the ecm_prep_reorg branch 2 times, most recently from 295bcab to d244b6f Compare April 11, 2025 23:06
@classmethod
def load_json(cls, filepath: Path) -> dict:
"""Loads data from a .json file
class ECMUtils:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This class is similar to ECMPrep, but ECMPrep is meant to store methods that update Measure and MeasurePackage instances. It might make sense to merge the two since they both contain common static methods used throughout the module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. I like having them separate as isolating the prepare_measures and prepare_packages functions is useful. I'm not sure that split_clean_data should be in ECMPrep vs. ECMUtils since the nature of that function seems more related to the latter.

Also, isn't it confusing to have a class named ECMUtils and a separate module named utils.py? Is there another candidate that would differentiate better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, I changed the location of split_clean_data and changed the class name to ECMPrepHelper

@aspeake aspeake force-pushed the ecm_prep_reorg branch 2 times, most recently from 12f148e to 121debc Compare April 17, 2025 22:59
@aspeake aspeake requested a review from jtlangevin April 17, 2025 23:00
@jtlangevin
Copy link
Collaborator

Thanks for the helpful summary @aspeake. Generally looks good but a few small questions, in addition to my comment above:

  • You note that New scout/ecm_prep_vars.py module, with class EPlusMapDicts previously in ecm_prep.py - is EPlusMapDicts in there? I can’t find it and thought you removed all the EPlus-related stuff.
  • The Measure fill_mkts function is quite long and can probably be refactored into a bunch of smaller functions, in addition to main(). Is that coming in a future PR?

@aspeake aspeake force-pushed the ecm_prep_reorg branch 3 times, most recently from 6b23ec1 to 14f0317 Compare July 14, 2025 16:23
@aspeake
Copy link
Collaborator Author

aspeake commented Jul 14, 2025

Thanks for the helpful summary @aspeake. Generally looks good but a few small questions, in addition to my comment above:

* You note that `New scout/ecm_prep_vars.py module, with class EPlusMapDicts previously in ecm_prep.py` - is `EPlusMapDicts` in there? I can’t find it and thought you removed all the EPlus-related stuff.

* The Measure fill_mkts function is quite long and can probably be refactored into a bunch of smaller functions, in addition to main(). Is that coming in a future PR?

EPlusMapDicts got removed with the other EPlus stuff, I just had not updated that comment.

Yes, those types of updates will come in future PR(s). I did not break up any functions or update logic here, only reorganized location of the methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restructure ecm_prep.py
2 participants