-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Add multivariate statisticals #56
Conversation
Finished update/completing of first functional module to run ordination (multivariate statistics), including plotting. Covered Issue #40
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work and looks like a substantial amount of code. I like the structure, documentation and that the code you've added has corresponding tests.
I've left a couple of relatively minor comments, all really about the sections of code which are commented out. I wonder if you could try the pip install -e .
approach (I give more detail in a comment further down) and see if that works out for you and avoids the need to have the manual adding of the path. If so, then please remove any code that is commented-out as much as possible.
Other than that, this looks very good. I really appreciate that you added a detailed description in ordination.md
so it will be on the docs website.
[like] Zech, A. (Alraune) reacted to your message:
________________________________
From: Robin Richardson ***@***.***>
Sent: Wednesday, February 19, 2025 1:44:54 PM
To: MiBiPreT/mibipret ***@***.***>
Cc: Zech, A. (Alraune) ***@***.***>; Assign ***@***.***>
Subject: Re: [MiBiPreT/mibipret] Add multivariate statisticals (PR #56)
CAUTION: This email originated from outside of Utrecht University. Do not click links or open attachments unless you recognize the sender and know the content is safe.
@raar1 requested changes on this pull request.
Great work and looks like a substantial amount of code. I like the structure, documentation and that the code you've added has corresponding tests.
I've left a couple of relatively minor comments, all really about the sections of code which are commented out. I wonder if you could try the pip install -e . approach (I give more detail in a comment further down) and see if that works out for you and avoids the need to have the manual adding of the path. If so, then please remove any code that is commented-out as much as possible.
Other than that, this looks very good. I really appreciate that you added a detailed description in ordination.md so it will be on the docs website.
________________________________
In examples/ex02_Amersfoort/example_02_amersfoort_ordination.py<#56 (comment)>:
@@ -0,0 +1,102 @@
+"""Unconstrained Ordination (PCA) with plot for Amersfoort data.
+
+Example of diagnostic plotting using ordination with contaminant data from Amersfoort site.
+
***@***.***: Alraune Zech
+"""
+
+# import sys
I assume this commented out code is to allow you to develop the python package locally? Generally we try to avoid committing (non-documenting) commented-out code, although I understand why you have this here.
The standard way to import a package you are working on locally is to pip install it with the -e flag (meaning "editable"). For example, from the project root directory (the directory that contains pyproject.toml then you can run:
pip install -e .
This means that import mibipret should work fine in your scripts, as python knows it is a local package that is under current development.
________________________________
In mibipret/analysis/reduction/ordination.py<#56 (comment)>:
@@ -6,10 +6,12 @@
"""
import numpy as np
-import pandas as pd
+
+# import pandas as pd
Again a minor comment, but usually we would remove any commented-out code before merging our changes.
________________________________
In mibipret/analysis/reduction/transformation.py<#56 (comment)>:
+from scipy.stats import zscore
+from mibipret.data.check_data import check_data_frame
+
+# import sys
+# path = '/home/alraune/GitHub/MiBiPreT/mibipret/mibipret/'
+# sys.path.append(path) # append the path to module
+from mibipret.data.names_data import setting_data
+from mibipret.data.set_data import compare_lists
+
+
+def filter_values(data_frame,
+ replace_NaN = 'remove',
+ drop_rows = [],
+ inplace = False,
+ verbose = False):
+ """Filtering values of dataframes for ordination to assure all are numeric.
It's great that you are providing this detailed and well-structured documentation
—
Reply to this email directly, view it on GitHub<#56 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AMQQU2FHTXHDDLXLMQIIY4L2QSDFNAVCNFSM6AAAAABW25RRLKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMRWHAYTONJRGU>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, thanks. I left two suggestions below (that should delete a couple of lines of commented code if you accept them) but I've approved the PR so that when you're done you can merge it :)
Co-authored-by: Robin Richardson <[email protected]>
Co-authored-by: Robin Richardson <[email protected]>
|
Add ordination analysis and plotting, add example for amersfoort data.