Code refactor - #21
Draft
Kitchi wants to merge 12 commits into
Draft
Conversation
The code to parse the CSV files and images already exists, but are primarily contained within the zernikeBeam class. They do not belong there, so I've started with moving the CSV file parsing into it's own class, and will follow with the image parsing. This also opens up the ability to auto-discover the location of the CSV file, since that will require knowledge of which telescope made the input image and selecting accordingly. Currently that logic gets setup after initializing the zernikeBeam class, which requires an input CSV - so it becomes a chicken and egg problem unless the image parser is broken out into it's own class.
Decoupling the disk IO and manipulating the resulting dataframe.
The earlier commit did not have any setter methods for the attributes, which has been fixed. Added some checks to test if the input is in the right format.
Added unit tests for CSVParser. Added pytest as a requirement in pyproject.toml
Bugfixes in the plumber command line script to account for changes to CSVParser.
The parsing logic has been moved from the independent function within `image.py` into the `ImageParser` class in `parsing.py`. The `image.py` module has been removed. The `ImageParser` class has also been incorporated into the command line `plumber` script.
This was previously in the zernikeBeam class, but does not belong there. Refactored the plumber command line script, and other associated backend modules (`parsing.py` and `zernike.py`) to reflect the addition of this new class.
The telescope type was not being determined correctly, leading to incorrect dish diameters and hence image sizes. This is now fixed, and has been verified to produce the right beams for MeerKAT.
Uses a frozen version of the CSV file to test against a template image that is known to be correct for Stokes I. The template image lives in plumber-data/ which is a submodule to this repo, and must be checked out for the tests to work.
test_sky was using the wrong relative path for the plumber-data repo, updated it.
Made some changes to `src/plumber/telescopy.py` to rename functions to be more appropriate, fixed `src/plumber/scripts/plumber.py` to reflect these changes, and added `test_metadata.py` to test the `TelescopeInfo()` class.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactoring the mammoth class in
zernike.pyto be more modular, and added several unit and integration tests. This PR is still active, the goal is to be able to discover the path of the coefficient CSV files internally without passing it into the command line, with the ability to override the default with a command line argument.I am opening the PR now for discussion on the refactor so further changes can be made as necessary.