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

Rename all the things (that are outdated/confusing/inconsistent) #37

Closed
eacharles opened this issue Jun 15, 2023 · 3 comments · Fixed by #47
Closed

Rename all the things (that are outdated/confusing/inconsistent) #37

eacharles opened this issue Jun 15, 2023 · 3 comments · Fixed by #47
Assignees
Labels
bug Something isn't working

Comments

@eacharles
Copy link
Collaborator

From Alex's PR

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 #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:

knnpz.py --> knnPZ.py
Inform_KNearNeighPDF --> KNNInformer
KNearNeighPDF --> KNNEstimator
NaiveStack --> NaiveStackSummarizer
Inform_NZDir --> NZDirInformer
NZDir.py --> nzDir.py
NZDir --> NZDirSummarizer
pointEstimateHist.py --> pointEst.py
PointEstimateHist --> PointEstSummarizer
Inform_PZFlowPDF --> PZFlowInformer
pzflow.py --> pzflowPZ.py
PZFlowPDF --> PZFlowEstimator
RandomPZ --> RandomPZEstimator
Inform_SimpleSOMSummarizer --> SimpleSOMInformer
sklearn_nn.py --> simpleNN.py
Inform_SimpleNN --> SimpleNNInformer
SimpleNN --> SimpleNNEstimator
Inform_somocluSOMSummarizer --> SOMocluInformer
somocluSOMSummarizer --> SOMocluSummarizer
Inform_trainZ --> TrainZInformer
TrainZ --> TrainZSummarizer
VarInferenceStack --> VarInfStackSummarizer

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.

@sschmidt23
Copy link
Collaborator

Looks like the wrong issue got linked, reopening.

@aimalz
Copy link
Collaborator

aimalz commented Jul 10, 2023

@joezuntz made a good suggestion on LSSTDESC/rail_gpz_v1#6:

Feel free to say "no" to this, but have you considered using a prefix to help namespace the stage classes? In TXPipe all our stages start with "TX" and that really helps a) make it obvious what is a pipeline stage and what's just a helper class b) distinguish from RAIL stages.

I know this is just my pedantry coming through so please do ignore this if you prefer!

Thanks! Yeah, as you can see, that's indeed one of the problems we're running into. We had a prefix for Informers (that predated the ceci refactoring so was a bit confusing for people new to stages) but were inconsistent with Estimators/Summarizers (some ended in PDF, Summarizer, etc. but some were just the name of the method without specification). Is the proposal to always make it a prefix rather than a suffix?

@joezuntz
Copy link
Contributor

Prefix is maybe a little more traditional (all the MacOS classes start with NS, for example), but suffix should work too, sure.

@aimalz aimalz added the bug Something isn't working label Jul 14, 2023
@aimalz aimalz closed this as completed Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants