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

Feat: support array type for datetime_ref in plot_age_pyramid() #43

Conversation

Vincent-Maladiere
Copy link
Collaborator

@Vincent-Maladiere Vincent-Maladiere commented Mar 30, 2023

Description

Checklist

  • If this PR is a bug fix, the bug is documented in the test suite.
  • Changes were documented in the changelog (pending section).
  • If necessary, changes were made to the documentation (eg new pipeline).

strayMat and others added 7 commits December 8, 2022 09:09
…iffers for each patient: eg. the date of their inclusion in the study.
- add a new test for pytest.raise
- add a parsable datetime string
- add better error messages when bad value or bad type
Co-authored-by: Thomas Petit-Jean <[email protected]>
@Vincent-Maladiere Vincent-Maladiere changed the title plot_age_pyramid: add the possibility to have one datetime_ref that d… Feat: support array type for datetime_ref in plot_age_pyramid() Mar 30, 2023

person_["age"] = deltas / (365 * 24 * 3600)
person_ = person_.query("age > 0.0")
mask = person_["age"] > 90
Copy link
Contributor

@strayMat strayMat Mar 31, 2023

Choose a reason for hiding this comment

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

Why this mask ?
Is it for privacy reason due to low samples with aged more than 90 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was mainly to gather rare categories (90, 100, 110) and avoid making too many bins with very few individuals. Does it seem relevant to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is relevant for me as I understand that this mask basically merge the bins: (90, 100], (100, 110], and above, which is the classic intention of a user.

@strayMat
Copy link
Contributor

strayMat commented Apr 6, 2023

I struggle with resolving the conversation but LGTM !

@Vincent-Maladiere
Copy link
Collaborator Author

I need to add some documentation then we'll be good to merge :)

@Vincent-Maladiere
Copy link
Collaborator Author

I'm just waiting for #41 to be merged so that I can complete the documentation!

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (24bf047) 83.80% compared to head (099b337) 83.84%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
+ Coverage   83.80%   83.84%   +0.03%     
==========================================
  Files          82       82              
  Lines        2488     2494       +6     
==========================================
+ Hits         2085     2091       +6     
  Misses        403      403              
Impacted Files Coverage Δ
eds_scikit/plot/age_pyramid.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Vincent-Maladiere Vincent-Maladiere merged commit 001fe9b into aphp:main Jun 1, 2023
@Vincent-Maladiere
Copy link
Collaborator Author

@strayMat finally, it's merged :)

@strayMat
Copy link
Contributor

strayMat commented Jun 1, 2023

So coole ! Thanks !

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.

3 participants