-
Notifications
You must be signed in to change notification settings - Fork 10
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
218 coda transforms #244
218 coda transforms #244
Conversation
example of using ALR, CLR & ILR from the package pyrolite.
arg for keeping the redundant column. Use existing exception classes where possible. Move some utility functions into more appropriate places.
testing notebook.
…mon to coda transforms.
…ion, normalization and some checks.
versions of simplex check and normalizing functions. Fix some issues discovered during writing tests.
For the purposes of testing that the functionality is correct, this paper was a good overall source (and the first source I found with a proper definition for pivot logratio transform), and this paper has a simple, follow-along example for CLR, ALR and ILR. |
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.
Hey! Overall code looks great and is easy to follow! My comments mostly concern some naming conventions and suggestion – feel free to disagree or counter-suggest something for these. I tried to check the different logratio logics too, but I have to admit I am not familiar with these methods myself.
denominator_column = df.columns[idx] | ||
columns = [col for col in df.columns] | ||
|
||
if not keep_redundant_column and denominator_column in columns: | ||
columns.remove(denominator_column) |
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.
If we decide to keep the denominator column, we will divide it by itself in the inner function, or am I wrong in here? This seems intuitively unintended to me, but I could be wrong
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.
Yup, that's the case. I thought there should be the option to keep the column even though it becomes "redundant", since I don't know what the actual use cases for the function are. But default behavior will still be to return a dataframe with one less columns than the input frame.
tests accordingly.
…ecorator to inverse_alr.
…s in VS code weren't working properly with chained decorator calls. Instead, perform the check within each function that needs it. Fix one of the compositional check tests that was resulting in a false positive assertion.
@nmaarnio All the suggested changes should now be addressed! |
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.
Thanks for the quick changes – looks really good to me now! I only had one comment/question about the alr_transform
parameterization.
column: The integer position based index of the column of the dataframe to be used as denominator. | ||
If not provided, the last column will be used. |
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.
Did you leave the type of this parameter int
on purpose? What I suggested was to let the user give the desired column name (str) instead of the index. This solution isn't bad, but can be a bit tricky if there are tens of columns in a DF :)
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.
I must have missed it or forgotten halfway through going through the change suggestions. 😄 It's now fixed!
…ete check_column_index_in_dataframe function as unused. Update notebook.
One more thing I noticed: the doc files are missing for these functions. After you have added them I will merge! |
I added doc files similar to those for other modules. But I couldn't get mkdocs serve or build to work as described in the instructions. I'm getting (the same issue is with all of them):
Is there something that needs to be done in addition to adding the files for them to work? |
You need to add an empty |
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.
LGTM, merging!
Implement the requested logratio transformations for compositional data (issue #218):