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

DM-36446: Add size vs magnitude plots to analysis_tools #65

Merged
merged 8 commits into from
Jun 22, 2023

Conversation

taranu
Copy link
Contributor

@taranu taranu commented Jan 26, 2023

This also includes a significant refactoring of matched difference plots/metrics so that they share some base classes.

@taranu taranu force-pushed the tickets/DM-36446 branch 3 times, most recently from 449e5e0 to a445d57 Compare January 26, 2023 21:12
@taranu taranu force-pushed the tickets/DM-36446 branch 2 times, most recently from 206a9e1 to 1abfedd Compare February 14, 2023 01:31
@taranu taranu force-pushed the tickets/DM-36446 branch 3 times, most recently from 7c5a55c to 77aa16c Compare March 2, 2023 01:11
@taranu taranu force-pushed the tickets/DM-36446 branch 7 times, most recently from 25a3534 to c52f5aa Compare May 16, 2023 14:17
@taranu taranu force-pushed the tickets/DM-36446 branch 2 times, most recently from ff7ef83 to fcd5022 Compare May 20, 2023 03:51
@taranu taranu force-pushed the tickets/DM-36446 branch 2 times, most recently from 2d28fd9 to 5cc64b8 Compare June 5, 2023 18:52
Comment on lines 31 to 32
"ExponentiateFromBaseVector",
"ExponentiateToPowerVector",
Copy link
Contributor

Choose a reason for hiding this comment

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

"Exponentiate"? I realize this is the correct usage of the word here, but it feels overly verbose. Can we agree to use ExpFromBaseVector and ExpToPowerVector and pretend that "Exp" stands for "Exponentiate"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"exp" implies e^x, which is not what this is. The other alternative I considered was RaiseToPower. I didn't like that partly because raise is a Python keyword, and I'm not sure if RaiseFromBase makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exponentiate is a confusing term, I think (at least, it is to me). I slightly prefer RaiseToPower and RaiseFromBase, but I accept your point that Raise is a protected word in other contexts. Although perhaps that's not an issue here?


compute_chi = Field[bool](
default=False,
doc="Whether to compute scaled flux residuals (chi)" " instead of magnitude differences",
Copy link
Contributor

Choose a reason for hiding this comment

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

Badly formatted double "".



class FluxConfig(Config):
"""Configuration for a flux vector to be loaded and potentially plotted"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing period.

setattr(target, name, action(*args, **kwargs))

def _set_flux_default(self, attr):
"""Set own config attr to appropriate string flux name"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing period.

shape_slot=SizeConfig(key_size="shape_{suffix}", name_size="Shape slot radius"),
)
size_type = ChoiceField[str](
doc="The type of size to calculate",
Copy link
Contributor

Choose a reason for hiding this comment

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

Here (and other config fields), missing period. However, if a few slip through, I'm not overly concerned.

)

def _check_attr(self, name_size: str):
"""Check if a buildAction has already been set"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing period.

@taranu taranu force-pushed the tickets/DM-36446 branch 3 times, most recently from 665d1c9 to 2d7d3c7 Compare June 21, 2023 22:18
@taranu taranu merged commit 737c078 into main Jun 22, 2023
7 checks passed
@taranu taranu deleted the tickets/DM-36446 branch June 22, 2023 18:24
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.

None yet

2 participants