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

naming consistency/clarity within src/rail/estimation #17

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

aimalz
Copy link
Contributor

@aimalz aimalz commented Jul 10, 2023

Change Description

This PR (and other concurrent PRs in the other repos) include renamings of modules and stages within src/rail/estimation for consistency and transparency to users as outlined in LSSTDESC/rail#37. (Expect a few more PRs to address the smaller set of necessary consistency/clarity changes outside src/rail/estimation using the same branch.)

  • My PR includes a link to the issue that I am addressing

I request that multiple reviewers please do not hesitate to suggest changes as needed! The goals are consistency, clarity, and longevity, so if something looks unclear, inconsistent, or insufficiently flexible to accommodate future development, now is the time to make adjustments.

Solution Description

The following changes were made across all the rail repos, along with updates to the contributing documentation (which should be propagated to the rail python project template so prompts regarding naming are included in future PR checklists).

files:
  delightPZ.py --> delight_hybrid.py
  GPz.py --> _gpz_util.py
  knnpz.py --> k_nearneigh.py
  naiveStack.py --> naive_stack.py
  NZDir.py --> nz_dir.py
  pointEstimateHist.py --> point_est_hist.py
  pzflow.py --> pzflow_nf.py
  randomPZ.py --> random_gauss.py
  simpleSOM.py --> minisom_som.py
  sklearn_nn.py --> skl_neurnet.py
  somocluSOM.py --> somoclu_som.py
  trainZ.py --> train_z.py
  varInference.py --> var_inf.py
classes:
  Inform_BPZ_lite --> BPZliteInformer
  BPZ_lite --> BPZliteEstimator
  Inform_CMNNPDF --> CMNNInformer
  CMNNPDF --> CMNNEstimator
  Inform_DelightPZ --> DelightInformer
  delightPZ --> DelightEstimator  
  Inform_GPz_v1 --> GPzInformer
  GPz_v1 --> GPzEstimator
  Inform_FZBoost --> FlexZBoostInformer
  FZBoost --> FlexZBoostEstimator
  Inform_KNearNeighPDF --> KNearNeighInformer
  KNearNeighPDF --> KNearNeighEstimator
  NaiveStack --> NaiveStackSummarizer
  NZDir --> NZDirSummarizer  
  Inform_NZDir --> NZDirInformer
  PointEstimateHist --> PointEstHistSummarizer
  Inform_PZFlowPDF --> PZFlowInformer
  PZFlowPDF --> PZFlowEstimator
  RandomPZ --> RandomGaussEstimator
  Inform_SimpleNN --> SklNeurNetInformer
  SimpleNN --> SklNeurNetEstimator
  Inform_SimpleSOMSummarizer --> MiniSOMInformer
  SimpleSOMSummarizer --> MiniSOMSummarizer
  Inform_somocluSOMSummarizer --> SOMocluInformer
  somocluSOMSummarizer --> SOMocluSummarizer
  Inform_trainZ --> TrainZInformer
  TrainZ --> TrainZEstimator
  VarInferenceStack --> VarInfStackSummarizer   

Code Quality

  • I have read the Contribution Guide
  • My code follows the code style of this project
  • My code builds (or compiles) cleanly without any errors or warnings
  • My code contains relevant comments and necessary documentation

Bug Fix Checklist

  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

Given that we are still pre-v1, I have not explicitly included backward compatibility, but a block of import X as Y can be constructed from the above list of changes.

Other Change Checklist

  • I have updated the tutorial to highlight my new feature (if appropriate)
  • I have updated unit/End-to-End (E2E) test cases to cover any changes

I fixed some instances of outdated descriptions in the demo notebooks, but others require more substantial editing, e.g. when they describe code that has long since been removed from the demo in question or aspects of the API that have significantly changed since the descriptions were written. The demos require a thorough review before v1 that's out of scope for this series of PRs.

@aimalz aimalz requested a review from sschmidt23 July 10, 2023 14:30
@aimalz aimalz self-assigned this Jul 10, 2023
@aimalz
Copy link
Contributor Author

aimalz commented Jul 13, 2023

I think the test failure isn't related to anything I changed. @sschmidt23 Can you confirm whether that's true?

@sschmidt23
Copy link
Contributor

I wasn't sure what caused the test failure, looks like it's not finding h5py for some reason, so the pyproject.toml made need updating, given the big reorganization.

@sschmidt23
Copy link
Contributor

Oh, it's because there's an outstanding PR that should be merged in first, but I was having some trouble getting the coverage to work for some reason. I asked @eacharles to have a look at that coverage issue, once we get that PR merged we'll need to rebase this PR with that one, then everything should work.

@aimalz
Copy link
Contributor Author

aimalz commented Jul 14, 2023

Thanks @sschmidt23 for looking into it! Should I take this to mean I should hold off on merging all these PRs and making the releases until that's resolved?

@sschmidt23
Copy link
Contributor

Delight is pretty self-contained (it's not used in any demos or anything), and it's not actively being used by anyone currently. If this repo is the only holdout then I think it would be fine to merge everything else and we can track this down next week or at the RAIL-fest the following week.

@aimalz
Copy link
Contributor Author

aimalz commented Jul 14, 2023

Okay, thanks, that makes sense! I'm going to hit all the buttons and try not to break anything. . .

@aimalz aimalz added bug Something isn't working help wanted Extra attention is needed labels Oct 18, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4707895) 0.00% compared to head (cecde80) 0.00%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #17   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          1       1           
  Lines          2       2           
=====================================
  Misses         2       2           
Flag Coverage Δ
unittests 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sschmidt23 sschmidt23 left a comment

Choose a reason for hiding this comment

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

I commented out the pylint command that was choking, as once changes are propagated to the rail template we'll be replacing the workflow file with the updated one anyway.

@sschmidt23 sschmidt23 merged commit 5032962 into main Oct 24, 2023
5 checks passed
@sschmidt23 sschmidt23 deleted the user/aimalz/renaming branch October 24, 2023 18:02
@aimalz
Copy link
Contributor Author

aimalz commented Oct 30, 2023

Thank you @sschmidt23 for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants