Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clean and finalize removal of the basilisk dependencies #50

Merged
merged 39 commits into from
Feb 25, 2025
Merged

Clean and finalize removal of the basilisk dependencies #50

merged 39 commits into from
Feb 25, 2025

Conversation

jorainer
Copy link
Member

This PR:

  • cleans the package up and uses reticulate without basilisk.
  • includes a new (quarto) vignette with usage examples.
  • includes the spectra filtering methods from matchms.
  • improves the data translation between R and Python.
  • changes the similarity calculations to use only reticulate.

bittremieux and others added 25 commits February 5, 2025 10:33
@bittremieux
Minor typos and comment formatting
df87dc0
@bittremieux
Don't use basilisk context
8a411e5 
- Global import matchms.
- Operate on reticulate Python objects directly instead of using Python string scripts.
- Custom`r_to_py` for direct reticulate instead of `rspec_to_pyspec`.
- Removed some unused helper functions.
@bittremieux
Reticulate is used to manage Python dependencies directly
9a1ccdc 
To get started, users should do the following:
```
reticulate::install_python()
install_python_packages()
```
@bittremieux
Remove basilisk
0a0fdee
@bittremieux
Format with styler
ecebf9b
@bittremieux
Some formatting
138e226
@bittremieux
Minor simplification
7fd51ac
@bittremieux
4 spaces indentation
4e94c99
@bittremieux
Move onLoad to separate file
b944539 
Also use `assign` instead of `<<-` to assign the matchms import to the global namespace.
@bittremieux
Move spectrum conversion code
7e3c7c0
@bittremieux
Update namespace
c41faa6
@bittremieux
Fix import name
f9ebf38
@bittremieux
Update documentation to reflect removed spectraVariableMapping
e9222ec 
Because using `r_to_py.Spectra` can't take additional arguments.
@bittremieux
Move Python environment code
80ba9b8
@bittremieux
Reformat
b76ee08
@bittremieux
Support virtualenv, conda, and system Python
6de2ec1
@bittremieux
Remove basilisk configuration
90324ee
@bittremieux
Update dependency installation
413dddc 
- NumPy < 2.0
- Use pip in conda
@bittremieux
Add as contributor
d546d15
@jorainer
Merge pull request #38 from bittremieux/without_basilisk
11225e4 
Manage Python environment with reticulate instead of basilisk
@jorainer
Merge branch 'main' into without_basilisk
0ac5e6d
@jorainer
refactor: improve the r-to-Python translation of MS data
7ff7b72
@jorainer
refactor: finalize conversion unit tests and documentation
07dd046
@jorainer
refactor: complete removal of basilisk dependencies
fb90f85
@jorainer
docs: add vignettes and move previous documentation
5f497ba
@jorainer jorainer requested a review from mdgrv February 19, 2025 14:20
This was referenced Feb 19, 2025
Open
Closed
Closed
@jorainer jorainer requested a review from philouail February 20, 2025 07:51
@philouail
Copy link
Collaborator

philouail commented Feb 24, 2025 •
edited
Loading

I see you changed all the parameters names to remove the "Param" ending. Wouldn't it make more sense to follow the general naming of these kind of functions of RforMassSpectrometry ?

philouail
philouail approved these changes Feb 25, 2025
View reviewed changes
Copy link
Collaborator

@philouail philouail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No coding changes on my side, just small comments.

It must have been so much work, but it was worth it because this is really really cool ! Thanks

@jorainer
Copy link
Member Author

Thanks for the review! especially because it's so much at present. I'll fix and push the changes you requested. Regarding the naming of parameters Param not Param. I'm also not happy with it. During the hackathon we decided to drop the Param, so the parameter class matches the name of the python function/class. It looks a bit odd and is not following our general RforMassSpectrometry naming convention, but otherwise we would have ended up with names such as:

remove_peaks_around_precursor_mzParam or RemovePeaksAroundPrecusorMzParam() - we thus agreed to use just the name of the Python function instead...

@jorainer jorainer merged commit 8b30d92 into main Feb 25, 2025
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philouail philouail philouail approved these changes

@mdgrv mdgrv Awaiting requested review from mdgrv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jorainer @philouail @bittremieux