Open cisaacstern opened 2 years ago
@alisonpchase, made your earlier comment ☝️ into this new Issue for organizing our conversation.
If I understand correctly, the function get_water_iops
from your earlier Matlab code represents the approach you would like to now replicate in Python. I've copied that function below (click its name in the list below to expand the code).
I hope its okay to copy sections of the Matlab code here (if not, let me know here or offline, and I can remove). This was the easiest way I could think to structure a conversation around next steps.
The hierarchical bullets below get_water_iops
represent what I believe are all the other functions called from within this function, all of which we will need to replace with Pythonic versions in order to get our IOPs routine to work. If the sub-function is a link, that means its a builtin Matlab function, and the link goes to what I believe is the corresponding place in the Matlab documentation. If it's an expandable button, then it's a custom function, and the relevant code is copied within the expanded content.
My first question for you is: does the below organization seem accurate? Does the list below accurately reflect the full extent of what we will need to translate into Python, in order to implement your IOPs methods? If not, please let me know what I've overlooked or misunderstood.
get_water_iops
interp1
isnan
betasw124_ZHH2009
RInw
BetaT
rhou_sw
dlnasw_ds
PMH
find
rad2deg
NaN
size
cos
rad
tempsal_corr
Let's make sure the bulleted list above is indeed an accurate reflection of the work ahead. If so, I'll preview my suggestion that the next thing we work on are function definitions and docstrings for each of these custom functions. An example would be,
# Matlab function definition line:
# `function [a_sw,bb_sw] = get_water_iops(wave,T,S)`
# Python definition and docstring
def get_water_iops(wave, T, S):
"""Description of what this function does.
Parameters
----------
wave: (input type for wave?)
Description of wave.
T: (input type for T?)
Description of T.
S: (input type for S?)
Description of S.
Returns
-------
a_sw: (return type for a_sw?)
Description of a_sw.
bb_sw: (return type for bb_sw?)
Description of bb_sw.
"""
pass # this function doesn't do anything yet, so we're just going to `pass` for now
These definitions could go into a new module, e.g. pigments_from_rrs/iops.py
.
@alisonpchase, one more design note for now. The repo you found provides a good example for how to manage reference vectors in a Pythonic way. That is, store the data itself as CSVs, and then assign it to a capitalized constant with pd.read_csv
, as in, e.g.: https://github.com/tadz-io/hydropt/blob/master/hydropt/bio_optics.py
This is more idiomatic for Python than defining the vectors within the function bodies, as you've done in the Matlab functions. It's also more flexible and maintainable: this way we'll be able to swap out those reference vectors without touching the functions themselves.
In terms of specific implementation, though, it looks like importlib.resources
is a more contemporary way of managing this than pkg_resources
(the approach used in tadz-io/hydropt/
).
@cisaacstern delayed big thanks for this framework to get going with the water iops functions. I will work on the function definitions in a new module, maybe pigments_from_rrs/water_iops
? Should I create a PR for this? Or just work on the code and commit it to repo? I think my understanding is that the PR isn't necessary since it's my own repo, but can be a nice way for us to keep track of something we are collaborating on?
It should be fine to have the copied matlab code posted as long as we are referencing the papers. Currently the references are mentioned in the comments of the higher level functions - we can make sure we have complete references in our docstrings maybe, as we get the functions organized.
maybe pigments_from_rrs/water_iops? Should I create a PR for this? Or just work on the code and commit it to repo? I think my understanding is that the PR isn't necessary since it's my own repo, but can be a nice way for us to keep track of something we are collaborating on?
Yep, pigments_from_rrs/water_iops
sounds like a perfect spot for it.
Good question re: PRs, to which the answer is: definitely make a PR for this. 😄 While it is strictly true that you can commit changes directly to this repo, you are 100% correct that in our collaborative context, the PR makes it infinitely easier to keep track of what the new material is. Even if you were working completely on your own, using PRs is still a good way to compartmentalize changes to your code.
Good luck with this! And definitely let me know how I can help as you go along (no question too small!).
Thanks!! sounds good. So to check my workflow: I checkout a new branch, work on my .py module of functions, commit it, then open a PR on it to have a decided space for us to collaborate on it?
Yep! Let's say your new branch is called iops
.
Once you've committed some code to your local branch, you can
git push origin iops
To push that branch to a new branch of the same name on GitHub.
Then GitHub should prompt you to open a PR from the new branch. Once the PR is open, all commits pushed to the associated branch will be tracked in the PR.
@cisaacstern I'm diving back in, and looking at the next step in the data processing for our next incremental PR. A key step in the inversion of Rrs to get information (in our case, pigments) is defining the absorption and scattering spectra of water itself, so that we can account for and remove that signal from the measured light reflectance spectra.
This is a standard activity in optical oceanography computing, so I did a search and found some other GitHub repos where people have worked on this. In particular, there is this one: https://github.com/tadz-io/hydropt which is a full inversion model. In their case, they define a basis vector for all phytoplankton absorption, and where my method differs is that I separate out the absorption by different pigments. Anywho, they do have some code that defines the "IOPs" (Inherent Optical Properties, i.e. the absorption and scattering spectra) for "clear_nat_water". My matlab code uses the temperature and salinity of seawater to adjust the IOP spectra, so it may be different, but in case it's useful we could look at how they did the inversion of Rrs.
So I think our next PR should be to define the IOPs of water, which are known, but vary as a function of temperature and salinity, so maybe we can pass the temperature and salinity info in somewhere.
Originally posted by @alisonpchase in https://github.com/alisonpchase/pigments-from-rrs/issues/8#issuecomment-999001962