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

generalize fit() for non Ia applications #56

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Conversation

AmandaWasserman
Copy link
Collaborator

Generalize so that fit() can be used for other instances than Ia/non Ia.

Still need to work on ideas for a multi-class abilitiy.

Copy link

github-actions bot commented Oct 18, 2024

Before [477015a] After [db778fa] Ratio Benchmark (Parameter)
195M 197M 1.01 benchmarks.peakmem_learn_loop('KNN')
132±1ms 132±1ms 1.01 benchmarks.time_feature_creation
188M 188M 1 benchmarks.peakmem_learn_loop('RandomForest')
148±1ms 149±1ms 1 benchmarks.time_learn_loop('KNN', 'RandomSampling')
2.57±0.02s 2.56±0.02s 1 benchmarks.time_learn_loop('RandomForest', 'UncSampling')
153±0.8ms 151±1ms 0.98 benchmarks.time_learn_loop('KNN', 'UncSampling')
2.60±0.01s 2.56±0.01s 0.98 benchmarks.time_learn_loop('RandomForest', 'RandomSampling')

Click here to view all benchmarks.

Copy link
Collaborator

@drewoldag drewoldag left a comment

Choose a reason for hiding this comment

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

Overall I think this looks fine. I left a couple of docstring suggestions. Nothing critical.

src/resspect/fit_lightcurves.py Outdated Show resolved Hide resolved
additional_info: list = []):
"""
Perform fit to all objects from a generalized dataset.

Parameters
----------
data_dic: str
Dictionary containing the photometry for all light curves.
data_dic: list
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
data_dic: list
data_dic: list[dict]

Minor point - doesn't need to be fixed in this PR - but since data_dic is now expected to be a list, it might make sense to simply call it data.

@drewoldag drewoldag merged commit c683c9b into main Oct 30, 2024
7 checks passed
@drewoldag drewoldag deleted the generalize-features branch October 30, 2024 18:59
drewoldag added a commit that referenced this pull request Oct 30, 2024
* generalize fit() for non Ia applications

* Update src/resspect/fit_lightcurves.py

Co-authored-by: Drew Oldag <[email protected]>

---------

Co-authored-by: Drew Oldag <[email protected]>
drewoldag added a commit that referenced this pull request Oct 31, 2024
* WIP - Initial work to implement pluggable feature extractors - Unit tests are not passing, need to make a thorough scan of the code for lowercase references to feature extractors. i.e. "bazin" vs. "Bazin".

* Initial commit for pluggable query_strategies. (#59)

* generalize fit() for non Ia applications (#56)

* generalize fit() for non Ia applications

* Update src/resspect/fit_lightcurves.py

Co-authored-by: Drew Oldag <[email protected]>

---------

Co-authored-by: Drew Oldag <[email protected]>

* Attempted to clean up references to default values of `bazin` and replaced with `Bazin`.

* Introduce logic to ease the transition from lower case to upper case feature extractor names. Adding tests.

---------

Co-authored-by: AmandaWasserman <[email protected]>
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.

2 participants