Skip to content

Conversation

@vineetbansal
Copy link
Contributor

Stripped off pandas from the rest of the codebase.

@vineetbansal vineetbansal marked this pull request as draft April 9, 2023 23:01
@codecov
Copy link

codecov bot commented Apr 9, 2023

Codecov Report

Merging #911 (5c089b7) into vb/src_ascopy (e98d09d) will increase coverage by 0.18%.
The diff coverage is 92.82%.

@@                Coverage Diff                @@
##           vb/src_ascopy     #911      +/-   ##
=================================================
+ Coverage          88.18%   88.37%   +0.18%     
=================================================
  Files                119      119              
  Lines              10118    10448     +330     
=================================================
+ Hits                8923     9233     +310     
- Misses              1195     1215      +20     
Impacted Files Coverage Δ
src/aspire/commands/cov3d.py 0.00% <0.00%> (ø)
src/aspire/commands/denoise.py 0.00% <0.00%> (ø)
src/aspire/commands/preprocess.py 0.00% <0.00%> (ø)
src/aspire/source/image.py 91.83% <95.23%> (+0.86%) ⬆️
src/aspire/storage/starfile.py 90.00% <95.45%> (+2.74%) ⬆️
src/aspire/commands/extract_particles.py 86.79% <100.00%> (ø)
src/aspire/ctf/ctf_estimator.py 98.00% <100.00%> (-0.02%) ⬇️
src/aspire/denoising/class_avg.py 74.41% <100.00%> (+0.19%) ⬆️
src/aspire/denoising/denoised_src.py 92.59% <100.00%> (+0.28%) ⬆️
src/aspire/source/coordinates.py 92.30% <100.00%> (+0.09%) ⬆️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vineetbansal vineetbansal marked this pull request as ready for review April 10, 2023 00:42
@vineetbansal
Copy link
Contributor Author

vineetbansal commented Apr 10, 2023

A lot of the acrobatics happening in this branch have to do with the fact that:

  • some code is accessing the _metadata attribute directly and is thus tied to its implementation.
  • It's a bit tricky to get the nth row of each value of a dict of lists, merge two such dicts etc, hence some code with dict comprehension, zip etc.
  • The relion interop code was written to differentiate between scalar values and vectors on the basis of the incoming object being a DataFrame. Now that they're all dicts, one has to peek at the values.

Probably a good project for someone down the line would be to have a Metadata class to contain some of the ugliness here. On the other hand, the relion code is dealing with general-purpose 2D data and is not relying on it coming from metadata of an ImageSource, so gains might be limited.

I've submitted this as a separate PR so it can be merged after vb/src_ascopy is merged, or before - depending on how much disruption is acceptable, and the resources available to review and test these more thoroughly.

Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

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

Thanks @vineetbansal , tried running this against a known bad file (10028 shiny particles), and ASPIRE still seems to indicate it is bad, so I'm happy with that. I think this is probably ready for Joakim to also sign off on.

There were a few small things, but I think they are just questions and minor cosmetic followups we can do. Thanks!

@garrettwrong
Copy link
Collaborator

Looks like the redundant PR merge conflicted themselves.

@garrettwrong
Copy link
Collaborator

Docs build failed because EMDB ftp was offline yesterday ( and so we could not download our datafile). Its online now, restarting jobs.

Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Much cleaner, although I confess there are some list comprehension acrobatics here that I have trouble following (in the aspire.source.* modules). Perhaps some can be simplified (or commented) but this is not too urgent to hold up a merge.

@garrettwrong garrettwrong requested a review from janden April 14, 2023 18:16
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants