-
Notifications
You must be signed in to change notification settings - Fork 26
ImageSource.save Optics Block #1337
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
base: develop
Are you sure you want to change the base?
Conversation
garrettwrong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks. Couple minor things noted.
Can you add a test that covers slicing the source? I don't think arbitrary slicing will mess up your logic, but it might in the future. Better to add test now while it works :D.
If you haven't let's make sure you can convert some real STAR files that we use. (Read them in and write them out in a re-usable way, ideally with the user doing nothing...). If we can't, that is going to be a problem and I'm not sure we could take the code yet. This may be fine already, so don't interpret it as a code criticism 😇 .
Can we document in a tutorial an example that shows generating a realistic Simulation, saving with this new format, and loading with Relion? (This can be a non-running, display only, example). I see your testing that we can load one back ourselves, but, we're probably not the target application...
Thanks!
src/aspire/source/image.py
Outdated
| optics_block["_rlnOpticsGroup"] = [] | ||
| optics_block["_rlnOpticsGroupName"] = [] | ||
| for field in optics_value_fields: | ||
| optics_block[field] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. Try using a defaultdict(list) for optics_block and see if that reduces some of this code.
Fact check me, but I believe all Python dicts are now naturally ordered for versions of Python we support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's cleaner. Thanks.
Yep, you're correct about dicts being ordered.
| if "_rlnImageSize" not in metadata: | ||
| metadata["_rlnImageSize"] = np.full(self.n, self.L, dtype=int) | ||
|
|
||
| if "_rlnImageDimensionality" not in metadata: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what in the world
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
src/aspire/source/relion.py
Outdated
| metadata = RelionStarFile(self.filepath).get_merged_data_block() | ||
|
|
||
| # Promote legacy _rlnAmplitude column to the ASPIRE-specific name. | ||
| if "_rlnAmplitude" in metadata and "_aspireAmplitude" not in metadata: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate you are trying to make old files forward compatible. Nice idea, but perhaps too dangerous. Consider if Relion decides tomorrow they want to use _rlnAmplitude ...
How much stuff is actually going to break if we do not do this...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not going to break anything. I just had it in for compatibility with old files. Removed.
src/aspire/utils/relion_interop.py
Outdated
| # of certain key fields used in the codebase, | ||
| # which are originally read from Relion STAR files. | ||
| relion_metadata_fields = { | ||
| "_aspireAmplitude": float, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't the whole point that _aspireAmplitude doesn't belong in relion_metadata_fields 😇 ?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. Removed.
| np.testing.assert_equal(src.amplitudes.dtype, dtype) | ||
|
|
||
| # offsets are always stored as doubles | ||
| # offsets and amplitudes are always stored as doubles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
| sim.save(starpath, overwrite=True) | ||
|
|
||
| star = RelionStarFile(str(starpath)) | ||
| assert star.relion_version == "3.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, are we exactly making 3.1 only, or some later version or ? What versions are possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really 3.1 up to the current version. The format has stayed the same, with the added optics block, since 3.1.
| np.testing.assert_array_equal(optics["_rlnImageSize"], np.full(kv_ct, res)) | ||
| np.testing.assert_array_equal(optics["_rlnImageDimensionality"], np.full(kv_ct, 2)) | ||
|
|
||
| # Due to Simulation random indexing, voltages will be unordered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is true. You should have the order of filters in the simulation. I think this ordering would follow from that one if I understand correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "unordered" is the wrong word here. The optics["_rlnVoltage"])'s will be in the order they were encountered during save. That will not necessarily be the same ascending order the voltages were in when we created the ctf filters (which get applied randomly to the images).
|
|
||
|
|
||
| @pytest.mark.parametrize("batch_size", [1, 6]) | ||
| def test_simulation_save_sets_voxel_size(tmp_path, batch_size): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😇
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1337 +/- ##
===========================================
+ Coverage 90.65% 90.68% +0.02%
===========================================
Files 134 134
Lines 14431 14487 +56
===========================================
+ Hits 13082 13137 +55
- Misses 1349 1350 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
934a2cd to
a3d6381
Compare
…-generated dummy values when loading. Unit test.
Save
ImageSource's with Relion >= 3.1 convention for starfiles, with separatedata_opticsanddata_particleblocks.Some other additions in this PR are:
amplitudesin doubles_rlnAmplitudemetadata field to_aspireAmplitude, since_rlnAmplitudeis not a valid Relion field nameImageSourcemrcs files with pixel/voxel size in header. Some Relion command line tools extract pixel size (angpix) from here._rlnImageSizeand_rlnImageDimensionality