add spectral residuals - #41
Conversation
vatsal-j
left a comment
vatsal-j
left a comment
There was a problem hiding this comment.
Mainly need to update docstrings for changed functions
|
Also, there aren't any conflicts but should sanity check the |
|
Given recent PRs, we can now rebase and remove the Manifest.toml. @marchett2 , can you support that along with handling the @vatsal-j 's review comments, or do you need a hand? |
|
Yes, I will handle the docstring updates asap. Vatsal will help with Manifest.toml. |
|
Just pushed changes to update docstrings (except for unmix.jl, since I didn't see any). Also in this update: --Addition of nnls as another optimization option in unmix_pixel Let me know if I missed anything. |
vatsal-j
left a comment
vatsal-j
left a comment
There was a problem hiding this comment.
Most of this looks good, and thank you for the docstring updates, they also help me to understand functionally what's changing! As I understand it, the main changes are related to unmix_line() returning the spectral residuals. To my eye it looks good, I assume this has been tested and works?
One minor question is why we check the length of results in write_line_results() for the spectral residuals? If we're always outputting spec residuals from unmix_line() without a flag controlling it, they should always be present in results, otherwise something is wrong, right?
There are also a couple other features being added here. For future reference, these should definitely have their own separate PRs so they are easier to track.
The NNLS addition looks fine. Are the Glob and HDF5 packages related to that too?
The change to prepare_options() also looks fine to me, but worth asking if subsampling should even be an option if we run out of memory, or if it should just do it by default if class number > 7 (also, is there a better way of doing this down the line?). The argument handling also needs to be better (i.e. prepare_options() needing a seed value even if subsample == false.
There are some other comments I've thrown in that are minor and I can take care of when I rebase and resolve conflicts. After that I think this should be fine to merge, especially if we know the new functionality works and is useful in the ongoing fractional cover work, with the caveat that this will break some backwards compatibility (like in the example notebook) and might open some more issues down the line (which is fine).
| - A tuple containing: | ||
| - `refl./ norm::Matrix{Float64}`: A matrix of scaled reflectance data with same dimensions as `refl`. | ||
| - `norm::Matrix{Float64}`: A matrix of normalization coefficients corresponding to `refl`. Is used to get | ||
| unscaled coefficients for spectral residuals. If criteria == "none", the 1 is returned as normalization |
There was a problem hiding this comment.
Minor typo here (should be "then* 1 is returned..."). I can fix.
| EllipsisNotation = "da5c29d0-fa7d-589e-88eb-ea29b0a81949" | ||
| GDAL = "add2ef01-049f-52c4-9ee2-e494f65e021a" | ||
| Glob = "c27321d9-0574-5035-807b-f59d2c89b15c" | ||
| HDF5 = "f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f" |
There was a problem hiding this comment.
Maybe I missed where they are being used in this update, but why are we adding Glob and HDF5 to the package?
There was a problem hiding this comment.
I added these b/c I have other scripts (to run validation and simulation unmixing) that I created but never checked in. I just used this environment to run these scripts. Feel free to delete, and I will figure out how to get a new environment going (I am still very unfamiliar with Julia).
There was a problem hiding this comment.
Sounds good. I also have been having a time trying to grasp Julia's environment and package management 😄
There was a problem hiding this comment.
@marchett2 Is JLLWrappers also a package we can remove from the Project.toml?
|
RE: The change to prepare_options() also looks fine to me, but worth asking if subsampling should even be an option if we run out of memory, or if it should just do it by default if class number > 7 (also, is there a better way of doing this down the line?). The argument handling also needs to be better (i.e. prepare_options() needing a seed value even if subsample == false. Yes for both. I can make the change for the seed, but I will leave the decision to you and Phil if you'd like to change this function for class number > 7. |
Ok sounds good, for this case does it make sense to make the seed an optional argument and use a default value? Or does having a default seed sort of defeat the purpose. @pgbrodrick thoughts on this? Also the subsampling option vs mandatory thing? We can (and should) probably move this discussion to a separate issue as well. |
|
RE: prepare_options. I ended up simply moving the seed activation inside the if statement such that it will only be used when subsample = True. We can keep the default seed value. I swapped the two parts of the if statement for subsampling to be cleaner (if subsample ... else). |
|
Perfect! |
| end | ||
|
|
||
| # Prepare combinations if relevant | ||
| if subsample |
There was a problem hiding this comment.
@marchett2 - This looks like it will get handled within unmix_pixel - can you help me with the rationale for pulling it up here (I recall there was one, just not seeing it clearly here when reviewing)? Is it for simulations or something else? If so, we should remove from unmix_pixel so that it's not redundant.
|
Hey All, thanks for the forward progress here. Lets go ahead and just raise an exception if the class number is > 7 for now, and toss in an issue for good ways to pre-compute & generalize this (we should be able to estimate the array size and terminate if it gets to big in advance...but lets save that for separate. Subsampling needs to be retained...threw in an inline comment, I'm not sure right now why exactly it's getting moved. |
3 participants
No description provided.