Parametrises column names #269

ns-rse · 2024-03-14T15:40:17Z

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...

pgfinder/pgio.py
pgfinder/validation.py
pgfinder/matching.py

...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...

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

Other changes

Tidying up doc strings switching to the newer format of str | Path rather than the older Union[str, Path]. This can be supported under Python 3.8 with from __future__ import annotations but Python 3.8 is due to reach End of Life in 2024-10-31.
Drop support for Python 3.8 as it will reach End of Life later this year and set the minimum version to 3.9 and adding support/testing for 3.11.

Tidying up doc strings we are switching to the newer format of `str | Path` rather than the older `Union[str, Path]`. This can be supported under Python 3.8 with `from __future__ import annotations` but since Python 3.8 is due to reach End of Life in [2024-10-31](https://endoflife.date/python) there is no harm in dropping support now and setting the minimum version to 3.9 and adding support/testing for 3.11.

+ Documentation on output columns added. + Parametrises columns in `lib/pgfinder/config/columns.yaml` and loads these on initialisation. + Uses the columns defined and loaded in `pgfinder.find_pg` functions for loading ftrs and maxquant files. + Adds a dictionary for mapping and renaming consolidated variables. Abstracting the information out of hard-coded variables into parameter files simplifies the process of extending to future update file formats and makes managing changes (still to come in this PR) easier. Will have to add to documentation how the configuration file works and perhaps also something to change it, although the way things are going I don't think anyone will use this interactively and so this documentation would be for developers (which doesn't make it any less important as the more we can help our future selves the better).

+ Updates `pgfinder/matching.py` to use the parametrised fields names. + Annotates `pgfinder/config/columns.yaml` with comments. + Updates regression test files (column headers are the only thing that has changed) + Updates tests for consolidation (column headers are the only thing that has changed) + Removes comment from test.

TheLostLambda · 2024-03-14T16:28:02Z

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 · 2024-03-14T17:45:31Z

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.

Seems I was mistaken about the minimum Python version for the newer method of writing type hints.

TheLostLambda · 2024-03-14T17:49:08Z

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

codecov · 2024-03-14T17:50:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

@@            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

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

TheLostLambda · 2024-03-14T18:55:10Z

@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 · 2024-03-15T08:18:00Z

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 · 2024-03-18T17:37:58Z

@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 · 2024-03-18T18:24:52Z

@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 · 2024-03-18T18:26:36Z

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

TheLostLambda

Overall looks good, with just a few bits of documentation that I could answer questions for, some commented bits of code that should probably be deleted, and a non-essential note or two!

I'll approve if you want to tidy those last little bits and then merge things in!

lib/docs/data_dictionary.md

lib/pgfinder/matching.py

TheLostLambda · 2024-03-18T18:44:12Z

lib/pgfinder/matching.py

            ascending=[True, False],
            key=lambda k: abs(k) if is_numeric_dtype(k) else k,
            inplace=True,
            kind="stable",
        )
        group.reset_index(drop=True, inplace=True)

+        # The hard coded 'Delta ppm' and 'Intensity column names aren't yet handled dynamically


It seems somewhat similar with the "Inferred structure ..." and "Intensity ..." ones which are only partially dynamic. If this is a faff to sort out though, then there is no need to worry about it for now

I didn't address these as it would have required making pgfinder.input a dictionary so that the Intensity column could be referenced and would have required another entry under pgfinder to hold the value of Delta ppm.

I'll do those in a separate PR though tomorrow as there is no point in leaving the job half-done. Thanks for picking me up on this.

@TheLostLambda

Thanks for feedback from @TheLostLambda Also ordered pre-commit hooks.

slackline added 4 commits March 13, 2024 10:29

Update validation functions to work with parametrised column names

aa0e854

ns-rse added the io Input and Output label Mar 14, 2024

ns-rse requested review from TheLostLambda, bobturneruk and Ankur-AVP-Patel March 14, 2024 15:40

[pre-commit.ci] Fixing issues with pre-commit

819e6d8

Minimum Python version 3.10

4d49244

Seems I was mistaken about the minimum Python version for the newer method of writing type hints.

ns-rse force-pushed the ns-rse/267-column-names branch 2 times, most recently from 6298479 to 4d49244 Compare March 15, 2024 12:02

TheLostLambda approved these changes Mar 18, 2024

View reviewed changes

Improving dictionary and removing print statements

2cf0534

Thanks for feedback from @TheLostLambda Also ordered pre-commit hooks.

ns-rse mentioned this pull request Mar 19, 2024

Further parametrisation of column names #273

Closed

ns-rse merged commit 736694b into master Mar 19, 2024
9 of 10 checks passed

ns-rse deleted the ns-rse/267-column-names branch March 19, 2024 07:41

ns-rse mentioned this pull request Mar 20, 2024

ci: Tests fail under Python 3.11 and 3.12 on Windows #278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parametrises column names #269

Parametrises column names #269

ns-rse commented Mar 14, 2024

TheLostLambda commented Mar 14, 2024

ns-rse commented Mar 14, 2024

TheLostLambda commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

TheLostLambda commented Mar 14, 2024

ns-rse commented Mar 15, 2024 •

edited

Loading

TheLostLambda commented Mar 18, 2024

ns-rse commented Mar 18, 2024

TheLostLambda commented Mar 18, 2024

TheLostLambda left a comment

TheLostLambda Mar 18, 2024

ns-rse Mar 19, 2024

Parametrises column names #269

Parametrises column names #269

Conversation

ns-rse commented Mar 14, 2024

Other changes

TheLostLambda commented Mar 14, 2024

ns-rse commented Mar 14, 2024

TheLostLambda commented Mar 14, 2024 • edited Loading

codecov bot commented Mar 14, 2024 • edited Loading

Codecov Report

TheLostLambda commented Mar 14, 2024

ns-rse commented Mar 15, 2024 • edited Loading

TheLostLambda commented Mar 18, 2024

ns-rse commented Mar 18, 2024

TheLostLambda commented Mar 18, 2024

TheLostLambda left a comment

Choose a reason for hiding this comment

TheLostLambda Mar 18, 2024

Choose a reason for hiding this comment

ns-rse Mar 19, 2024

Choose a reason for hiding this comment

TheLostLambda commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

ns-rse commented Mar 15, 2024 •

edited

Loading