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

Reading from csv files #364

Merged
merged 14 commits into from
Mar 5, 2024
Merged

Reading from csv files #364

merged 14 commits into from
Mar 5, 2024

Conversation

williamjameshandley
Copy link
Collaborator

@williamjameshandley williamjameshandley commented Mar 1, 2024

Description

Following conversations with @yallup, this PR implements reading csv files which have been written with the .to_csv method. Getting this right for all variations on labels/weights being dropped is a little fiddly, and the current implementation prioritises robustness over speed (reading multiple times in some cases), but will function as a low-latency start point.

For consistency this also moves WeightedLabelledPandas into its own file anesthetic.weighted_labelled_pandas.py rather than anesthetic.samples, which reduces clutter for users just looking at the latter file.

Checklist:

  • I have performed a self-review of my own code
  • My code is PEP8 compliant (flake8 anesthetic tests)
  • My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
  • New and existing unit tests pass locally with my changes (python -m pytest)
  • I have added tests that prove my fix is effective or that my feature works
  • I have appropriately incremented the semantic version number in both README.rst and anesthetic/_version.py

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3ea992c) to head (0fec842).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #364   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        36    +2     
  Lines         2979      3043   +64     
=========================================
+ Hits          2979      3043   +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@williamjameshandley
Copy link
Collaborator Author

@yallup does this perform as expected?

Copy link
Collaborator

@yallup yallup left a comment

Choose a reason for hiding this comment

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

Just tested and the functionality seems good. Including read_csv into the read_chains logic chain may be useful and then plugs into the gui automatically to boot?

@williamjameshandley
Copy link
Collaborator Author

I agree this is a good idea, but leaves a small choice:

If we want it to be consistent with read_chains (read_polychord etc), then this should really take a root rather than a filename (root.csv), which differs from the pandas default read_csv.

Thoughts/preferences?

@williamjameshandley
Copy link
Collaborator Author

(could always accept both)

@yallup
Copy link
Collaborator

yallup commented Mar 4, 2024

I agree this is a good idea, but leaves a small choice:

If we want it to be consistent with read_chains (read_polychord etc), then this should really take a root rather than a filename (root.csv), which differs from the pandas default read_csv.

Thoughts/preferences?

I was going to remark (before checking that it is actually consistent with pandas as is), that root as a filename arg would be preferable. Although we override pandas I think there is enough local precedent to make this conform to the "root" style chains file reading.

@williamjameshandley
Copy link
Collaborator Author

williamjameshandley commented Mar 5, 2024

OK, I've implemented both, so you can pass root or root.csv to anesthetic.read.csv.read_csv. The gui and read_chains should now both work with csv files.

Copy link
Collaborator

@yallup yallup left a comment

Choose a reason for hiding this comment

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

Does everything I was looking for, am not testing any of the intricacies around labels but looks good to me(rge)

@williamjameshandley williamjameshandley merged commit 24cbfa5 into master Mar 5, 2024
20 of 22 checks passed
@williamjameshandley williamjameshandley deleted the read_csv branch March 5, 2024 10:09
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.

2 participants