Add option to omit field numbers

[sjrothfuss]

I hope to use this package in Python STOPGAP but it is imperative that the star files produced (e.g. motls, tomolists, parameters) are compatible with legacy MATLAB STOPGAP. The files produced by this package are incompatible with MATLAB STOPGAP in a couple ways. One of which is the inclusion of field numbers after field names, i.e., a field name in the loop block is rendered as _fieldname #n.

This PR creates the include_field_num option to omit that number such that the field name would be just _fieldname. include_field_num is optional and defaults to True for back compatibility.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.79%. Comparing base (5b19406) to head (1b6035d).
Report is 16 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
+ Coverage   80.93%   82.79%   +1.85%     
==========================================
  Files           7        7              
  Lines         278      308      +30     
==========================================
+ Hits          225      255      +30     
  Misses         53       53              

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

[jojoelfe]

Thanks for the PR! That looks all super reasonable. Would you mind renaming the option include_field_number? @alisterburt was quite keen on having very explicit names

[sjrothfuss]

Also sounds reasonable! I decided to do include_field_numbers to match plurality of quote_all_strings. Sorry for the multiple commits.

[jojoelfe]

Cool, thanks so much! I'll leave this open for 24h in case somebody else wants to chime in and otherwise merge and release tomorrow.

[alisterburt]

I'm on vacation so don't have time to look at the spec etc - I'm not super keen on complicating the API here if STOPGAP isn't compliant and the field numbers are part of the spec, in that case I would prefer that you maintain a simple fork/depend on the lower level functionality within this package

Let me know your thoughts!

[sjrothfuss]

As far as I can tell, field numbers are not part of the spec. I could be wrong but none of the examples I have seen (Wikipedia, original publication, and 2021 conference abstract) use them.

Feel free to wait to confirm/deny until you're done with vacation.

[jojoelfe]

The only part of the spec is that everything after # is ignored.

Except if contained within a text string, a single sharp signals that the characters up to the end of the line are used for comment only.

That means that technically STOPGAP is not following the spec by not ignoring them. But, my feeling is that this is not something that really complicates the API, but gives you more control about what is written. So I would merge it.

[alisterburt]

Huh okay, I've internalised a RELION-ism it seems - let's merge here!

[sjrothfuss]

Oh, I didn't think of it as comment handling. I can ask Will about updating STOPGAP to check for sharp-delimited comments.

[jojoelfe]

Yes, cisTEM also adds these little headers over the values using the sharp-comment, which is kinda nice when looking at them:

loop_
_cisTEMAnglePsi #1
_cisTEMAngleTheta #2
_cisTEMAnglePhi #3
_cisTEMDefocus1 #4
_cisTEMDefocus2 #5
_cisTEMDefocusAngle #6
_cisTEMScore #7
_cisTEMPixelSize #8
_cisTEMMicroscopeVoltagekV #9
_cisTEMMicroscopeCsMM #10
_cisTEMAmplitudeContrast #11
_cisTEMBeamTiltX #12
_cisTEMBeamTiltY #13
_cisTEMImageShiftX #14
_cisTEMImageShiftY #15
_cisTEMOriginalImageFilename #16
_cisTEMOriginalXPosition #17
_cisTEMOriginalYPosition #18
#   PSI   THETA     PHI      DF1      DF2  ANGAST   SCORE    PSIZE    VOLT      Cs    AmpC  BTILTX  BTILTY  ISHFTX  ISHFTY                             ORIGINAL_IMAGE_FILENAME  ORIGX ORIGY 
 330.00   81.00  275.25  12791.4  11218.3   68.96   11.21  2.00015  300.00    2.70  0.0700   0.000   0.000   0.000   0.000 /home/elferich/tpt/s_00120_15.0_May06_02.21.55_121_auto.mrc  1440.11  2606.20 

In any case, I still think it's worthwhile adding these options if they help for some cases without interfering with the default behavior.

[jojoelfe]

After sleeping on it I think it's up to @sjrothfuss whether to go ahead: If you think this is not necessary because the comment handling can be included in STOPGAP, we should probably avoid adding the extra code, as minimal as it is. But if you think this will help to smooth things over with existing versions of STOPGAP, let's merge.

[sjrothfuss]

Thanks @jojoelfe. Let me talk to Will about why STOPGAP doesn't handle field name comments.

[sjrothfuss]

Will and I agreed we'd add comment handling to STOPGAP motl reading. Comments are actually already implemented in other star reading, just hadn't been added to motl reading since STOPGAP motls weren't expected to have comments.

[sjrothfuss]

To summarize as regards this package, since field numbers are actually just line comments whether to include them (like RELION) or not (like STOPGAP) is just a style decision. Since AB is keen to keep options few, this package should only offer one and I don't see a convincing reason to change what is done currently.

Adding package credits to the top of every file does seem a little tacky but that's a different issue. 😉

[alisterburt]

@sjrothfuss is a holdover from when things were less robust, was useful for debugging issues! Happy to discuss on an issue/accept PRs to change the behaviour 🙂