LSSTDESC / rail_attic

Redshift Assessment Infrastructure Layers
MIT License
14 stars 9 forks source link

Issue/288/standardize naming #391

Open aimalz opened 1 year ago

aimalz commented 1 year ago

This is a draft PR for LSSTDESC/rail_base#1 so I can check that I'm not breaking the tests (because fitsio isn't behaving on my local machine today). The scope of changes is to standardize naming of classes (and hopefully their config parameters, although I'm not sure I understand them all well enough to do that alone) that are going to be isolated into standalone repos in LSSTDESC/RAIL#384, to be accompanied by PRs in the existing standalone repos as necessary. To prevent myself from getting distracted from the task at hand, I'm also adding TODO notes to give concrete starting points for LSSTDESC/rail_hub#22.

EDIT: Overall, the changes here pertain solely to consistency in class and module naming; apologies if this was unclear to anyone! The purpose is so we don't inadvertently ask users to guess at the syntax for a given stage with respect to the module it's in (occasionally risking namespace issues if they import common packages with the same as a RAIL module) and with respect to other stages that do similar things with other underlying algorithms, across creation and evaluation as well as within estimation (leaving open the possibility to distinguish algorithms that can be used for both estimation and summarization). It also addresses a common source of confusion among users trying to understand the idea of stages in general, due to naming standards we established before switching to ceci as a back-end: rather than being a function or method, if a stage is an object that takes an extra line of code to run, why are they sometimes named as actions or underlying algorithms (without indication of whether it's an estimator or summarizer)?

In any case, this PR affects formatting only, not content/meaning of any of the classes in question, so my hope is that the disruption is entirely degenerate with that anticipated for LSSTDESC/RAIL#384, if they happen at the same time. The changes include the following (just in rail.estimation for now):

I can put together a block of import X as Y for backwards compatibility in case anyone finds the changes too hard to propagate through pipelines you've already written.

Note that the base classes are unaffected (though they will be in LSSTDESC/rail_base#14, at least there aren't any Evaluators outside base RAIL), so stages that are not updated for consistency won't be broken. Propagation of consistent naming to the already standalone repos will be completed in subsequent PRs in those repos. However, I'd like to take advantage of the opportunity posed by the isolation-refactoring disruption to make similar changes to some of the degraders, on a separate issue/branch/PR akin to LSSTDESC/rail_base#14.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f3086e8) 100.00% compared to head (7bdb83f) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main LSSTDESC/RAIL#391 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 40 38 -2 Lines 2876 2763 -113 ========================================== - Hits 2876 2763 -113 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `100.00% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC) | Coverage Δ | | |---|---|---| | [src/rail/estimation/summarizer.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9zdW1tYXJpemVyLnB5) | `100.00% <ø> (ø)` | | | [src/rail/core/algo\_utils.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvY29yZS9hbGdvX3V0aWxzLnB5) | `100.00% <100.00%> (ø)` | | | [src/rail/estimation/algos/naiveStack.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9hbGdvcy9uYWl2ZVN0YWNrLnB5) | `100.00% <100.00%> (ø)` | | | [src/rail/estimation/algos/simpleNN.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9hbGdvcy9zaW1wbGVOTi5weQ==) | `100.00% <100.00%> (ø)` | | | [src/rail/estimation/algos/simpleSOM.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9hbGdvcy9zaW1wbGVTT00ucHk=) | `100.00% <100.00%> (ø)` | | | [src/rail/estimation/algos/somocluSOM.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9hbGdvcy9zb21vY2x1U09NLnB5) | `100.00% <100.00%> (ø)` | | | [src/rail/estimation/algos/trainZ.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9hbGdvcy90cmFpbloucHk=) | `100.00% <100.00%> (ø)` | | | [src/rail/estimation/estimator.py](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC#diff-c3JjL3JhaWwvZXN0aW1hdGlvbi9lc3RpbWF0b3IucHk=) | `100.00% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/LSSTDESC/RAIL/pull/391/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=LSSTDESC)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

aimalz commented 1 year ago

@yanzastro @sschmidt23 While propagating naming consistency through the demos I noticed that there are two seemingly slightly different versions of the somoclu demo. Can you confirm which I should remove (or update the markdown text to indicate the big picture differences I failed to find)?

EDIT: Related question for @sschmidt23 : Since both SimpleSOM and somocluSOM contained the same _computemagcolordata function, I moved it to src/rail/core/algo_utils.py. Then I discovered more very similar functions, make_color_data in sklearnNN and _computecolordata in knnpz. I think these are equivalent so all the algos using this functionality could call a single version of this function in src/rail/core/algo_utils.py (or possibly in a new src/rail/estimation/utils.py module instead) but they are indeed calculating the color-plus-one-mag information differently. Can you advise on whether this assessment is accurate and, if so, which version would be preferable?