Added new SkylineCmd arguments for building ion mobility libraries: -… #2301
Conversation
…-ionmobility-create-library and --ionmobility-create-library-name
|
Here's an example script: REM Tidy up any previous run first |
|
Here's the new documentation:
|
pwiz_tools/Skyline/CommandArgs.cs
Outdated
| @@ -337,6 +348,8 @@ private bool ValidateMinimizeResultsArgs() | |||
| return true; | |||
| } | |||
|
|
|||
| public MsDataFileUri IMSDbFile { get; private set; } | |||
There was a problem hiding this comment.
Too many capitals in the naming. Use ImsDb or Imsdb instead, like MsDataFile. We prefer title-case even for acronyms.
pwiz_tools/Skyline/CommandArgs.cs
Outdated
| @@ -362,6 +375,11 @@ private bool ValidateMinimizeResultsArgs() | |||
| public double? LockmassTolerance { get; private set; } | |||
| public LockMassParameters LockMassParameters { get { return new LockMassParameters(LockmassPositive, LockmassNegative, LockmassTolerance); } } | |||
|
|
|||
| private void CreateIMSDB(NameValuePair pair) | |||
| { | |||
| IMSDbFile = new MsDataFilePath(pair.ValueFullPath); | |||
There was a problem hiding this comment.
This is not an MsDataFile so strange to be using MsDataFilePath.
There was a problem hiding this comment.
Yup, I'll fix that, thanks.
| @@ -387,6 +393,9 @@ | |||
| <data name="CommandArgs_GROUP_IMPORT_Importing_results_replicates" xml:space="preserve"> | |||
| <value>Importing results replicates</value> | |||
| </data> | |||
| <data name="CommandArgs_GROUP_CREATE_IMSDB_Ion_Mobility_Library" xml:space="preserve"> | |||
| <value>Creating an ion mobility library</value> | |||
There was a problem hiding this comment.
What about adding to an existing IMSDB? Seems like that should also be possible.
There was a problem hiding this comment.
Agreed, that just wasn't the immediate requirement. If you agree I'll just open an issue for that and return my attention to Feature Finding.
There was a problem hiding this comment.
I think it is at least worth thinking through now how you would support that within the command argument group you have created. Maybe making this title something more generic like:
Ion mobility library support
And arguments maybe:
--ionmobility-library-add=path/to/file.imsdb
--ionmobility-library-create=path/to/file.imsdb
--ionmobility-library-name=name
And eventually:
--ionmobility-library-addresults
create assumes addresults, obviously. Though, a user might use the combination:
--ionmobility-library-add=path/to/file.imsdb --ionmobility-library-name=name --ionmobility-library-addresults
which might ultimately be what Scripps wants to do to create a single .imsdb with all of their compounds in it for sharing with other Skyline users.
There was a problem hiding this comment.
Agreed, the hierarchy should be --ionmobility-library-XXX rather than --ionmobility-create-XXX for best extensibility.
--ionmobility-create-library and --ionmobility-create-library-name are changed to --ionmobility-library-create and --ionmobility-library-name added a proper test various code tweaks to conform to our conventions
…library name argument. Also convert from AbstractFunctionalTestEx to AbstractUnitTestEx
…t directory Add more test conditions for commandline imsdb creation - now tests: normal operation support for not specifying "--ionmobility-library-name" error handling for illegal characters in imsdb filename error handling for non-existent subdirectories in imsdb file path error handling for specifying "--ionmobility-library-name" without "--ionmobility-library-create"
…//github.com/ProteoWizard/pwiz into Skyline/work/20220923_automate_imsb_creation
There was a problem hiding this comment.
Thanks for adding the new testing and the fix. I would like to see you try re-writing the test not to use a loop like the ones we were just discussing yesterday. These produce code that is unusual to most other developers and therefore harder to read and maintain. As a general concept, they allow more tightly coupled code to grow over time.
…ulture settings, including automatically determining the kind of separator in use
…//github.com/ProteoWizard/pwiz into Skyline/work/20220923_automate_imsb_creation
In particular move the DSV separator detection to the test support namespace and make it clear in comments that it isn't robust enough for general use.
- add optional expectedSuccess parameter to RunCommand which reports a mismatch with the logged output - added more testing to PerfCommandlineCreateImsDbTest mostly as another example of inspecting the output SrmDocument from a command-line run
…-ionmobility-create-library and --ionmobility-create-library-name