Skip to content
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

Feature step7 integration #53

Merged
merged 9 commits into from
Feb 2, 2024
Merged

Feature step7 integration #53

merged 9 commits into from
Feb 2, 2024

Conversation

kmilo9999
Copy link
Collaborator

  • Add analysis_db/B06_correct_separate_var.py for step 7
  • Cleaned code
  • Formatted file

Copy link
Member

@hollandjg hollandjg left a comment

Choose a reason for hiding this comment

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

Hey Camilo, thanks for this! I've one general question – was the removal of the "B2" regridded data throughout intentional? What's the source for that? Otherwise I've got a couple of comments.

Comment on lines 1 to 3
import os, sys


Copy link
Member

Choose a reason for hiding this comment

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

Looks like this bit is in the wrong place – all imports should be under the docstring

Base automatically changed from feat-step6-intg to main January 31, 2024 21:35
@kmilo9999
Copy link
Collaborator Author

Hey Camilo, thanks for this! I've one general question – was the removal of the "B2" regridded data throughout intentional? What's the source for that? Otherwise I've got a couple of comments.

We checked this with John in the dev meeting. The files should be compared against the basic-sliderule branch.

cpaniaguam
cpaniaguam previously approved these changes Feb 1, 2024
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

I say merge for now and refactor later.

If you like, consider moving all function definitions that happen in the middle of the script to the top just after the import statements. I also saw an import statement in the middle that might have slipped through the cracks. But I can do all of this later when implementing the cli!

Comment on lines +1 to +4
"""
This file open a ICEsat2 track applied filters and corections and returns smoothed photon heights on a regular grid in an .nc file.
This is python 3
"""
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the docstrings are not changing. @mochell, you might want to consider updating these.

src/icesat2_tracks/analysis_db/B06_correct_separate_var.py Outdated Show resolved Hide resolved
m slope of the fitted line
b intersect of the fitted line
"""
from scipy.ndimage.measurements import label
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move import to the top? In this case it's repeated so it can be deleted.

N_photons = np.zeros(GSUM.N_per_stancil.size)

counter = 0
for k, I in Gdict.items():
Copy link
Member

Choose a reason for hiding this comment

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

k may be unused in this loop.

Suggested change
for k, I in Gdict.items():
for _, I in Gdict.items():

Comment on lines 748 to 766
try:
io.save_pandas_table(
B2_v2, "B06_" + ID_name + "_B06_corrected_resid", save_path
) # all photos but heights adjusted and with distance coordinate
except:
os.remove(save_path + "B06_" + ID_name + "_B06_corrected_resid.h5")
io.save_pandas_table(
B2_v2, "B06_" + ID_name + "_B06_corrected_resid", save_path
) # all photos but heights adjusted and with distance coordinate

try:
io.save_pandas_table(
B3_v2, "B06_" + ID_name + "_binned_resid", save_path
) # regridding heights
except:
os.remove(save_path + "B06_" + ID_name + "_binned_resid.h5")
io.save_pandas_table(
B3_v2, "B06_" + ID_name + "_binned_resid", save_path
) # regridding heights
Copy link
Member

Choose a reason for hiding this comment

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

This can be refactored to something like this but it's tricky to tell it works as it should. To be safe better wait for test suite.

Suggested change
try:
io.save_pandas_table(
B2_v2, "B06_" + ID_name + "_B06_corrected_resid", save_path
) # all photos but heights adjusted and with distance coordinate
except:
os.remove(save_path + "B06_" + ID_name + "_B06_corrected_resid.h5")
io.save_pandas_table(
B2_v2, "B06_" + ID_name + "_B06_corrected_resid", save_path
) # all photos but heights adjusted and with distance coordinate
try:
io.save_pandas_table(
B3_v2, "B06_" + ID_name + "_binned_resid", save_path
) # regridding heights
except:
os.remove(save_path + "B06_" + ID_name + "_binned_resid.h5")
io.save_pandas_table(
B3_v2, "B06_" + ID_name + "_binned_resid", save_path
) # regridding heights
def save_table(data, tablename, save_path):
try:
io.save_pandas_table(data, tablename, save_path)
except Exception as e:
tabletoremove = save_path + tablename + ".h5"
print(e, f"Failed to save table. Removing {tabletoremove} and re-trying..")
os.remove(tabletoremove)
io.save_pandas_table(data, tablename, save_path)
B06_ID_name = "B06_" + ID_name
table_names = [B06_ID_name + suffix for suffix in ["_B06_corrected_resid", "_binned_resid"]]
data = [B2_v2, B3_v2]
for tablename, data in zip(table_names, data):
save_table(data, tablename, save_path)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this suggestion, pro-coding style. I dont see a reason why not to include it. I just did and it works as expected.

src/icesat2_tracks/analysis_db/B06_correct_separate_var.py Outdated Show resolved Hide resolved
Comment on lines +559 to +597
def reconstruct_displacement(Gx_1, Gk_1, T3, k_thresh):
"""
reconstructs photon displacement heights for each stancil given the model parameters in Gk_1
A low-pass frequeny filter can be applied using k-thresh

inputs:
Gk_1 model data per stencil from _gFT_k file with sin and cos coefficients
Gx_1 real data per stencil from _gFT_x file with mean photon heights and coordindate systems
T3
k_thresh (None) threshold for low-pass filter

returns:
height_model reconstucted displements heights of the stancil
poly_offset fitted staight line to the residual between observations and model to account for low-pass variability
nan_mask mask where is observed data in
"""

dist_stencil = Gx_1.eta + Gx_1.x
dist_stencil_lims = dist_stencil[0].data, dist_stencil[-1].data

gFT_cos_coeff_sel = np.copy(Gk_1.gFT_cos_coeff)
gFT_sin_coeff_sel = np.copy(Gk_1.gFT_sin_coeff)

gFT_cos_coeff_sel = gFT_cos_coeff_sel * tanh_fitler(Gk_1.k, k_thresh, sigma_g=0.003)
gFT_sin_coeff_sel = gFT_sin_coeff_sel * tanh_fitler(Gk_1.k, k_thresh, sigma_g=0.003)

FT_int = gFT.generalized_Fourier(Gx_1.eta + Gx_1.x, None, Gk_1.k)
_ = FT_int.get_H()
FT_int.p_hat = np.concatenate(
[-gFT_sin_coeff_sel / Gk_1.k, gFT_cos_coeff_sel / Gk_1.k]
)

dx = Gx.eta.diff("eta").mean().data
height_model = FT_int.model() / dx
dist_nanmask = np.isnan(Gx_1.y_data)
height_data = np.interp(
dist_stencil, T3_sel["dist"], T3_sel["heights_c_weighted_mean"]
)
return height_model, np.nan, dist_nanmask
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move function definitions to the top after the imports? To me it feels like they're in the way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can do a refactor by moving all this functions to a separate file, so B06_correct_separate_var only contains the logic applied to step 7. This can also be done to all the other steps.

Copy link
Member

Choose a reason for hiding this comment

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

I like this idea!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Just one more change I think you might have missed! I put it in a suggestion.

src/icesat2_tracks/analysis_db/B06_correct_separate_var.py Outdated Show resolved Hide resolved
@kmilo9999 kmilo9999 requested a review from cpaniaguam February 1, 2024 20:01
@kmilo9999 kmilo9999 merged commit 087ec34 into main Feb 2, 2024
1 check passed
@kmilo9999 kmilo9999 deleted the feat-step7-intg branch February 2, 2024 02:12
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