Skip to content

Conversation

@scholzri
Copy link
Contributor

@scholzri scholzri commented Feb 16, 2024

This pull request modifies the beer_lambert_law function to support partial pathlength factors per wavelength, as requested here by myself. The ppf's are now specified in a tuple and converted to a 2x1 array which than can be used in the equation.

@welcome
Copy link

welcome bot commented Feb 16, 2024

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

can you add / update a test @scholzri ? thx

----------
raw : instance of Raw
The optical density data.
ppf : float
Copy link
Member

Choose a reason for hiding this comment

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

this type is now wrong.

Copy link
Contributor Author

@scholzri scholzri Feb 16, 2024

Choose a reason for hiding this comment

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

Hi @agramfort , thank you for the hint, but I do not understand. Which type do you mean? Sorry, I am new to mne and python.

Copy link
Member

Choose a reason for hiding this comment

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

ppf is not a float anymore here. You make it a list and actually it should be a tuple

Copy link
Contributor Author

@scholzri scholzri Feb 16, 2024

Choose a reason for hiding this comment

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

I see. Thank you for the explanation! I changed it.
I also modified the test for beer lambert law.

@scholzri scholzri requested a review from agramfort February 17, 2024 15:48
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

We should keep backward compatibility with float, too, which is easy enough to do I think -- see suggested edits!

scholzri and others added 6 commits February 20, 2024 11:52
scholzri and others added 2 commits February 20, 2024 11:02
@scholzri
Copy link
Contributor Author

Thank you for the input @larsoner

@scholzri scholzri requested a review from larsoner February 20, 2024 11:08
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looks good other than one thing I think we need to revert. I'll commit the change to get CIs working on everything but let me know if you think it should be 6.0 in the example!

@larsoner larsoner merged commit abfa0a6 into mne-tools:main Feb 21, 2024
@welcome
Copy link

welcome bot commented Feb 21, 2024

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@larsoner
Copy link
Member

Thanks @scholzri !

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
…ambert law (mne-tools#12446)

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants