Refactor to create a AfidSet class #14

kaitj · 2023-08-23T13:57:09Z

Proposed changes
First pass refactor to include an AfidSet class in-time for the mini sprint. There are still a few things to iron out, but I think this should provide the basic model and some examples of needed methods (for json handling). The test cases at the moment are very basic (e.g. loading the existing valid fcsv file), but would like to include a mock AfidSet to use in tests in the future.

Types of changes
What types of changes does your code introduce? Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Other (if none of the other choices apply)

Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!

Changes have been tested to ensure that fix is effective or that a feature works.
Changes pass the unit tests
Code has been run through the poe quality task
I have included necessary documentation or comments (as necessary)
Any dependent changes have been merged and published

Notes
All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.

An `afids.py` file has been added in order to create an AFIDSet dict class, enabling the storing of both metadata information, well as loading fiducials from files directly into a dataframe. The choice of using a dataframe here is for easier downstream manipulation. - `get_afid` has been moved into here as a method of the AfidSet class Moved extension specific file handling into its own `.py` files (e.g. `extensions/fcsv.py`). This choice was made to simplify the maintenance of handling different file extensions. - Separate semi-private methods are used for grabbing the metadata, as well as loading the fiducials into a polars dataframe. Additionally `io.py` has been refactored to generalize the handling of different file types (calling the methods within the different `extensions`). Updates poetry.lock and pyproject.toml to include the `attrs` library

codecov · 2023-08-23T14:00:11Z

Codecov Report

Merging #14 (e13eebd) into main (c809d22) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##              main       #14    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            6         8     +2     
  Lines          150       348   +198     
==========================================
+ Hits           150       348   +198

Components	Coverage Δ
afids_utils/afids.py	`∅ <ø> (∅)`
afids_utils/ext	`100.00% <100.00%> (∅)`
afids_utils/transforms.py	`100.00% <ø> (ø)`

tkkuehn

This is looking good, just a few requests/questions

afids_utils/afids.py

afids_utils/io.py

afids_utils/afids.py

afids_utils/tests/test_io.py

tkkuehn · 2023-08-23T14:38:20Z

afids_utils/afids.py

+
+    slicer_version: str = attr.field()
+    coord_system: str = attr.field()
+    afids_df: pl.DataFrame = attr.field()


I think my preferred approach would be to define an Afid class with a label and x, y, z coords, then make this a list[Afid], and add a validator that it contains 32 Afids with labels in order.

Yes, we can certainly do that - it'd be similar to how we had it implemented in the validator. The validation logic is implemented, just not as a separate method (under load) currently. If we do switch to doing it that way, than I would say lets just get rid of the use of dataframes all together - I don't see foresee any use case for them and would save us an additional dependency.

Sounds good on all counts, with the note that I think it makes sense to implement the validation logic as an attrs validator to check the validation regardless of where the AFIDs are coming from

That works for me, with the validation logic currently in place, lets move the attrs validator as a thing to do during the sprint. Should be easy enough to take what is currently there.

Cool, this has been done now with the latest commit sans attrs validator.

- Remove `io.py`, moving methods to the AfidSet class. `load` is now a ClassMethod while `save` is a method of the instance. - Added an `AfidPosition` class, which is now used to store AFIDs - Drops polars from use, favouring `List[AfidPosition]` instead - Update mismatched desc in template.fcsv - Update strategies.py to generate coords using `AfidPosition` - Update all tests to correspond with changes made

kaitj · 2023-08-23T21:31:52Z

I think at this point, minus some of the additional stuff we talked about and barring any major changes - we should try to get this pushed in as the base code to build off of for the sprint.

kaitj added 8 commits August 22, 2023 08:17

add tests for handling fcsv extension

ddaea1b

first pass at refactoring io.py and tests

8c7dfee

first pass at refactoring afids.py and associated tests

485314e

change saving to use AfidSet

af9a15a

update code coverage

f8c2299

quality fixes and updates to exceptions thrown

df85426

update docstrings for afids_utils.io.save

bef365d

kaitj added the maintenance Updates or improvements that do not change functionality of the code label Aug 23, 2023

github-actions bot requested a review from tkkuehn August 23, 2023 13:57

github-actions bot assigned kaitj Aug 23, 2023

update doc requirements

29cdcef

tkkuehn requested changes Aug 23, 2023

View reviewed changes

kaitj added 5 commits August 23, 2023 11:09

update attr -> attrs

7cb62be

drop polars from dependency

8c51097

update codecov components

6001c00

update doc's toctree

c3fa441

kaitj force-pushed the maint/afids_class branch from 754cbaa to c3fa441 Compare August 23, 2023 20:57

kaitj added 4 commits August 23, 2023 17:09

minor refactor for better handling of load_fcsv

e5e83cd

add exceptions to the docs

6fa2177

linting / formatting updates

edd92be

fix doc rst eval

e13eebd

kaitj requested a review from tkkuehn August 23, 2023 21:31

kaitj mentioned this pull request Aug 24, 2023

Integrate automatic typehinting #12

Closed

kaitj linked an issue Aug 24, 2023 that may be closed by this pull request

Integrate automatic typehinting #12

Closed

kaitj mentioned this pull request Aug 24, 2023

Update to reusable workflows #13

Merged

9 tasks

tkkuehn approved these changes Aug 24, 2023

View reviewed changes

kaitj merged commit fa3e85f into afids:main Aug 24, 2023
7 checks passed

kaitj deleted the maint/afids_class branch August 24, 2023 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to create a AfidSet class #14

Refactor to create a AfidSet class #14

kaitj commented Aug 23, 2023 •

edited

codecov bot commented Aug 23, 2023 •

edited

tkkuehn left a comment

tkkuehn Aug 23, 2023

kaitj Aug 23, 2023

tkkuehn Aug 23, 2023

kaitj Aug 23, 2023 •

edited

kaitj Aug 23, 2023

kaitj commented Aug 23, 2023

Refactor to create a AfidSet class #14

Refactor to create a AfidSet class #14

Conversation

kaitj commented Aug 23, 2023 • edited

codecov bot commented Aug 23, 2023 • edited

Codecov Report

tkkuehn left a comment

Choose a reason for hiding this comment

tkkuehn Aug 23, 2023

Choose a reason for hiding this comment

kaitj Aug 23, 2023

Choose a reason for hiding this comment

tkkuehn Aug 23, 2023

Choose a reason for hiding this comment

kaitj Aug 23, 2023 • edited

Choose a reason for hiding this comment

kaitj Aug 23, 2023

Choose a reason for hiding this comment

kaitj commented Aug 23, 2023

kaitj commented Aug 23, 2023 •

edited

codecov bot commented Aug 23, 2023 •

edited

kaitj Aug 23, 2023 •

edited