Mesnage-Org / pgfinder

Peptidoglycan MS1 Analysis Tool
https://mesnage-org.github.io/pgfinder
GNU Lesser General Public License v3.0
4 stars 2 forks source link

Parametrises column names #269

Closed ns-rse closed 8 months ago

ns-rse commented 8 months ago

Closes #267

Parametrises the column headers for input files (different versions of ftrs and maxquant, theoretical masses) and the columns PGFinder renames these to and subsequently derived columns based on these.

These values were hard-coded at various places through out the code, this makes updating them awkward and fickle. Instead the values are defined in the file pgfinder/config/columns.yaml and this is loaded to a Python dictionary. The reference to these values are then used at various points rather than hard coded values themselves.

Required tweaks to...

...as well as some of the test files which were used for comparison (including the regression tests) where the header columns have changed.

This should in theory make it somewhat easier to...

  1. Extend to new file formats when ftrs version changes.
  2. Make it easier to change PGFinder column names if its decided the current are not sufficiently descriptive.

    Other changes

TheLostLambda commented 8 months ago

Moving columns to a config file has always felt like the ideal solution! Happy to give this a look soon — is there a reason tests are failing still?

ns-rse commented 8 months ago

Yes, I figured if I was going to change lots of things in lots of places it would be sensible to abstract it out and parameterise the values so its easier to change and extend in the future.

Tests passed locally (for once I checked before pushing and making the PR).

I'll have a look tomorrow.

TheLostLambda commented 8 months ago

Haha, just beat me to it! https://peps.python.org/pep-0604/ (minimum version of 3.10)

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.37%. Comparing base (329b88d) to head (4d49244). Report is 2 commits behind head on master.

:exclamation: Current head 4d49244 differs from pull request most recent head 2cf0534. Consider uploading reports for the commit 2cf0534 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #269 +/- ## ========================================== + Coverage 70.25% 72.37% +2.12% ========================================== Files 11 10 -1 Lines 511 496 -15 ========================================== Hits 359 359 + Misses 152 137 -15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TheLostLambda commented 8 months ago

@ns-rse Unfortunately looks like some Windows demons to slay — I've tried a couple of times to re-run it, but with no luck. I'm also stuck without a Windows machine to replicate it outside of the CI!

After that's ironed out, I'll review!

ns-rse commented 8 months ago

The error stems from pgfinder.gui.internal.uploaded_file() and the function with tempfile.TemporaryDirectory() as tempdir: these haven't been touched in the current Pull Request and work fine on other OS's so my crude guess at the moment is that it is something outside of this code base.

I don't see any way the changes this PR introduces could cause these errors, the with ... : statement is meant to implicitly close files when it is done using them, perhaps its an artifact of using tempdir "with"in the subsequent call to with open(file) as f: would be one possible cause at a guess but since it doesn't happen elsewhere its somewhat confusing.

TheLostLambda commented 8 months ago

@ns-rse Not sure how you'd like to carry on from here — I'd agree it doesn't seem to be the fault of this diff. Would you want to drop the 3.11 testing for Windows from the CI in the meantime?

ns-rse commented 8 months ago

@TheLostLambda Tricky, Python and OS versions are defined via matrixes, so changing that makes it fiddly. We could over-ride the merging rules of requiring all tests to pass and write it up as an issue to be addressed in due course perhaps.

TheLostLambda commented 8 months ago

@ns-rse I'm happy enough with that! I'll review this and we can get it merged then!