Skip to content

src(gates): Use cattrs + attrs #136

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

Merged
merged 3 commits into from
Sep 11, 2022
Merged

src(gates): Use cattrs + attrs #136

merged 3 commits into from
Sep 11, 2022

Conversation

gegnew
Copy link
Contributor

@gegnew gegnew commented Dec 14, 2021

Depends on #128 , #133 (will break until 128 is merged)
Fixes #118, #130
Supersedes #131 , since the underlying code changes quite a bit

Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

TODO: update the docs to show the correct/simplest usage pattern sometime.

Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Quick scan to start. Can you rebase next time you push please?

Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Sorry for being so slow!

Looking good, almost ready to land.

@gegnew gegnew force-pushed the ge/cattrs-gates branch 3 times, most recently from 4128181 to 7199a93 Compare January 19, 2022 16:03
@gegnew gegnew requested a review from zbjornson January 19, 2022 16:18
@gegnew gegnew force-pushed the ge/cattrs-gates branch 2 times, most recently from 9da4d58 to e1e4a2d Compare March 7, 2022 05:45
@gegnew gegnew requested review from zbjornson and removed request for zbjornson March 23, 2022 18:06
@gegnew gegnew requested a review from zbjornson March 23, 2022 19:41
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Much improved interface. A few more questions on it though.

Comment on lines 402 to 428
"""Create a RectangleGate.

Accepts all args and kwargs available for
[`RectangleGate.create()`][cellengine.resources.gate.RectangleGate.create].
"""
return RectangleGate.create(
self._id, *args, create_population=create_population, **kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Would it work to keep all of the named args and pass all of them to RectangleGate.create()? Know it's a lot of code, but it would make type info work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The args/kwargs, not the docstring, right? We certainly can, but it means we have to maintain them in two places.

The type info actually works for me; this is the hint I get:

def create_rectangle_gate(self, x_channel: str, y_channel: str, name: str, x1: float, x2: float, y1: float, y2: float, label: List[float]=[], gid: Optional[str]=None, locked: bool=False, tailored_per_file: bool=False, fcs_file_id: Optional[str]=None, fcs_file: Optional[str]=None, parent_population_id: str=None, /, *, create_population=True, x_channel: str, y_channel: str, name: str, x1: float, x2: float, y1: float, y2: float, label: List[float]=[], gid: Optional[str]=None, locked: bool=False, tailored_per_file: bool=False, fcs_file_id: Optional[str]=None, fcs_file: Optional[str]=None, parent_population_id: str=None) -> RectangleGate

Let me know what you think

Copy link
Member

@zbjornson zbjornson Sep 10, 2022

Choose a reason for hiding this comment

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

This won't be a breaking change if we change it later to include all the args here. What tool is giving you that type hint? VSCode/Pylance is just giving me this:
image

@gegnew gegnew force-pushed the ge/cattrs-gates branch 3 times, most recently from 3a85787 to 25cc0c2 Compare May 10, 2022 15:31
@gegnew gegnew requested a review from zbjornson May 12, 2022 11:33
@gegnew gegnew force-pushed the ge/cattrs-gates branch from 25cc0c2 to c75429a Compare May 23, 2022 07:13
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Merging #136 (7c59370) into master (0791214) will decrease coverage by 0.79%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   93.63%   92.83%   -0.80%     
==========================================
  Files          67       60       -7     
  Lines        2906     3084     +178     
==========================================
+ Hits         2721     2863     +142     
- Misses        185      221      +36     
Flag Coverage Δ
unittests 92.83% <87.50%> (-0.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cellengine/resources/fcs_file.py 89.18% <ø> (ø)
setup.py 0.00% <ø> (ø)
cellengine/resources/experiment.py 79.47% <62.79%> (-8.36%) ⬇️
cellengine/utils/api_client/APIClient.py 82.89% <64.81%> (-1.38%) ⬇️
cellengine/resources/gate.py 88.68% <87.80%> (+8.29%) ⬆️
cellengine/utils/helpers.py 75.67% <94.73%> (-9.08%) ⬇️
cellengine/resources/scaleset.py 92.15% <100.00%> (+0.49%) ⬆️
cellengine/utils/__init__.py 100.00% <100.00%> (ø)
cellengine/utils/api_client/BaseAPIClient.py 86.15% <100.00%> (ø)
cellengine/utils/parse_fcs_file_args.py 100.00% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gegnew gegnew force-pushed the ge/cattrs-gates branch from c75429a to 0d418f1 Compare June 16, 2022 15:41
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

RectangleGate is almost there. These comments apply to the other gate classes, but I didn't read through them yet.

@gegnew gegnew force-pushed the ge/cattrs-gates branch 2 times, most recently from 158ce7f to de34f53 Compare July 5, 2022 07:08
@gegnew gegnew force-pushed the ge/cattrs-gates branch from ff5efec to 097b08e Compare July 5, 2022 17:08
@gegnew gegnew requested a review from zbjornson July 5, 2022 17:10
Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Just one last suggestion and a typo.

Copy link
Member

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

I fixed up a bunch of stuff and pointed out a few of the changes below. All of gate.py has no type errors and all of the integration tests pass now. https://github.com/primitybio/cellengine-python-toolkit/compare/d672d65..8c957bb

(Saving test responses was a bad idea of mine here and in the R toolkit -- too much work to keep up with the changes and we don't catch errors when API changes break the toolkit. I just added GitHub action secrets for CellEngine credentials to let us run integration tests against the real API.)

@zbjornson zbjornson force-pushed the ge/cattrs-gates branch 2 times, most recently from 8c957bb to c4df360 Compare September 11, 2022 03:03
@zbjornson zbjornson merged commit 6099511 into master Sep 11, 2022
@zbjornson zbjornson deleted the ge/cattrs-gates branch September 11, 2022 03:06
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.

gate.parentPopulationId has been removed from the API Gate creation from experiment resource ignores the create_population parameter
3 participants