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-31175: Add online and offline blind solving code #27

Merged
merged 28 commits into from Dec 6, 2022

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

self.corrFile = corrFile
self._calcScatter() # sets self.scatterPixels and self.scatterArcseconds

def _buildWcs(self):
Copy link

Choose a reason for hiding this comment

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

Do you actually want this object to be mutable? I'm guessing not since you made buildWcs and calcScatter single-underscore and never call them outside init.

In that case, I suggest making this a frozen dataclass (from dataclasses import dataclass and add @dataclass(frozen=True)). Then, replace the existing class attributes with methods using @cached_property (from functools). The nice thing about that is that the wcs method will contain all but the last line of what is now _buildWcs, whereas plateScale will be a one-liner calling self.wcs.getPixelScale().asArcseconds(), and you won't need any single-underscore methods at all. Plus, if you really care about performance, if you never call those methods it won't bother to read the file and build the wcs in the first place.

Copy link

@taranu taranu left a comment

Choose a reason for hiding this comment

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

Mostly minor/inane/nitpicking comments; hopefully the others are useful.

python/lsst/summit/utils/astrometry/anet.py Show resolved Hide resolved
python/lsst/summit/utils/astrometry/anet.py Outdated Show resolved Hide resolved
raise RuntimeError(f"Could not find {binary} in path, please install 'solve-field' and either"
" put it on your PATH or specify the full path to it in the 'binary' argument")

def _writeConfigFile(self, wide):
Copy link

Choose a reason for hiding this comment

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

Just FYI, we are hopefully going to get documenteer/Sphinx support for type hints Soon and not have to write types in the numpy docstrings anymore. I'm not sure if that means we should do both in the meantime...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably that will need mypy on everything though, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly good to know though.

python/lsst/summit/utils/astrometry/anet.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/astrometry/anet.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/astrometry/utils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/astrometry/utils.py Show resolved Hide resolved
python/lsst/summit/utils/astrometry/utils.py Outdated Show resolved Hide resolved
python/lsst/summit/utils/astrometry/utils.py Show resolved Hide resolved
python/lsst/summit/utils/utils.py Outdated Show resolved Hide resolved
@mfisherlevine mfisherlevine merged commit 089f03b into main Dec 6, 2022
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