Feature step5 integration #51

kmilo9999 · 2023-12-28T23:03:14Z

Add step5 analysis_db/B04_angle.py
Updated ICEsat2_SI_tools/generalized_FT.py and ICEsat2_SI_tools/sliderule_converter_tools.py to fix integration issues
Cleaned and formatted files

cpaniaguam · 2024-01-12T15:10:16Z

src/icesat2_tracks/ICEsat2_SI_tools/generalized_FT.py

Modified in #45. Wait for merge?

cpaniaguam · 2024-01-12T15:10:36Z

src/icesat2_tracks/ICEsat2_SI_tools/sliderule_converter_tools.py

Modified in #45. Wait for merge?

…comments

… an old version. I updated it

…tion

Co-authored-by: Carlos Paniagua <[email protected]>

Fix step2

list installed depedencies

…-code fix: use slidegeneric access in sliderule

cpaniaguam

A couple more cleanups suggested!

.github/workflows/test-B01_SL_load_single_file.yml

Co-authored-by: Carlos Paniagua <[email protected]>

…2-tracks into feat-step5-intg

cpaniaguam

Happy to see this passing!

I have some general comments and questions, two suggestions for issues I've seen elsewhere in the package, and a couple suggested changes.

cpaniaguam · 2024-01-26T15:07:40Z

angle_optimizer.patch

What is this file for? Its extension is kind of confusing.

My idea was to patch the angle_optimizer which is the one making step5 crash. This patch file has the instruction that changes the specific line that causes the error. But it seems patching the file didnt work either. I'm removing it, but leaving a comment in the angle_optimizer.py file mentioning how we solved this issue.

cpaniaguam · 2024-01-26T15:11:05Z

src/icesat2_tracks/ICEsat2_SI_tools/angle_optimizer.py

+        # TODO: this funciton throws an error in CI. The nan_policy='omit' policiy was added to avoid this issue 
+        # according to the guidelines in  https://lmfit.github.io/lmfit-py/faq.html#i-get-errors-from-nan-in-my-fit-what-can-i-do
        self.fitter = self.LM.minimize(self.objective_func, self.params,  method=method,
                        args=fitting_args, kws=fitting_kargs ,
-                        nwalkers=self.nwalkers, steps=steps, pos= self.seeds, **kargs)
+                        nwalkers=self.nwalkers, steps=steps, pos= self.seeds,nan_policy='omit' , **kargs)


Is there something still left to do here? Otherwise, the 'TODO' part of the comment could be removed if the change is going to be kept.

This line is the one throwing the error. The TODO comment is just to remind us that something needs to be done here

cpaniaguam · 2024-01-26T15:14:03Z

src/icesat2_tracks/analysis_db/B02_make_spectra_gFT.py

+import icesat2_tracks.ICEsat2_SI_tools.spicke_remover as spicke_remover
+import datetime
+import icesat2_tracks.ICEsat2_SI_tools.generalized_FT as gFT
+from scipy.ndimage.measurements import label


I think this import throws a deprecation warning.

Suggested change

from scipy.ndimage.measurements import label

from scipy.ndimage import label

Maybe an issue should be created as I've seen this elsewhere in the package.

cpaniaguam · 2024-01-26T15:20:24Z

src/icesat2_tracks/analysis_db/B02_make_spectra_gFT.py

@@ -114,10 +118,12 @@ def linear_gap_fill(F, key_lead, key_int):
    exit()

 # test LS with an even grid where missing values are set to 0
+imp.reload(spec)


Lines with imp.reload and its corresponding import statements can be removed. imp or its successor importlib are modules people use during development, not for production code.

There are many instances where imp is used in the package. Maybe for another issue?

cpaniaguam · 2024-01-26T15:26:51Z

src/icesat2_tracks/analysis_db/B02_make_spectra_gFT.py

 for k in all_beams:
-    # print(k) # TODO: add logger
-    hkey = "heights_c_weighted_mean"
-    x = Gd[k + "/dist"][:]
-    # print(x[0], x[-1]) # TODO: add logger
+    print(k)
+    hkey = "h_mean"


L160 should go outside (before) of the for loop here.

Clarifying question for me: why the change "heights_c_weighted_mean" => "h_mean"?

Another sliderule breaking change perhaps?

@mochell any thoughts ? ☝️

L160 should go outside (before) of the for loop here.

@kmilo9999 What do you think of this change?

If this change is made, the other line, L191, where hkey is redefined with the same value can be deleted.

The hkey is being redefined in line 188 with the same value. I'm removing it from the loop

src/icesat2_tracks/analysis_db/B02_make_spectra_gFT.py

cpaniaguam

I noted a change that was undone (docstring at the top) and I saw a constant being defined in a for loop.

src/icesat2_tracks/analysis_db/B02_make_spectra_gFT.py

cpaniaguam · 2024-01-26T16:47:25Z

src/icesat2_tracks/analysis_db/B02_make_spectra_gFT.py

 for k in all_beams:
-    # print(k) # TODO: add logger
-    hkey = "heights_c_weighted_mean"
-    x = Gd[k + "/dist"][:]
-    # print(x[0], x[-1]) # TODO: add logger
+    print(k)
+    hkey = "h_mean"


L160 should go outside (before) of the for loop here.

@kmilo9999 What do you think of this change?

If this change is made, the other line, L191, where hkey is redefined with the same value can be deleted.

Co-authored-by: Carlos Paniagua <[email protected]>

cpaniaguam

🍻

kmilo9999 added 2 commits December 28, 2023 17:09

finished step5 integration

4a827a9

finshed step5 integration (forgot adding files to last commit)

86963a3

cpaniaguam reviewed Jan 12, 2024

View reviewed changes

src/icesat2_tracks/ICEsat2_SI_tools/generalized_FT.py Outdated

Copy link

Member

cpaniaguam Jan 12, 2024

Choose a reason for hiding this comment

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

Modified in #45. Wait for merge?

cpaniaguam reviewed Jan 12, 2024

View reviewed changes

chore: B04_angle.py -- organize imports, update file paths, add some …

ee8dfae

…comments

cpaniaguam mentioned this pull request Jan 12, 2024

Feature step6 integration #52

Merged

kmilo9999 added 9 commits January 15, 2024 22:42

merge with main

199e5ca

fixing errors with dependencies

2dc33dd

something happened with this generalized_FT_.py file. It came back to…

54cecd8

… an old version. I updated it

step5 not working here

65cc3da

updated make_spectra and generalize_FT to fix step2 and step5 integra…

f48f904

…tion

commented step5 test in the workflow

e78b79d

testing pipeline in phyton 311

b0a07f6

trying to run the steps on macos

b2f03a8

trying to run the steps on macos- fix mpi installation

94acbbf

Base automatically changed from feat-step4-intg to main January 17, 2024 18:58

kmilo9999 and others added 13 commits January 18, 2024 13:27

Apply suggestions from code review

53c9b9b

Co-authored-by: Carlos Paniagua <[email protected]>

fix: typo

9f0ebf5

Merge pull request #68 from brown-ccv/fix-step2

5cfccc6

Fix step2

uncommenting step5 in workflow

3c17664

specifying depdencies versions in toml file

1df4452

rolling back to dependencies specific versions

23c1c62

Update test-B01_SL_load_single_file.yml

889c5d7

list installed depedencies

Update test-B01_SL_load_single_file.yml

e218668

fix: use generic access in sliderule

fdc1946

Update sliderule client

e80e308

Merge pull request #75 from brown-ccv/34-remove-passwords-from-source…

6b3c289

…-code fix: use slidegeneric access in sliderule

specifying dependencies versions

072eccb

removed code, uncommenting workflows jobs

adca754

kmilo9999 requested a review from cpaniaguam January 24, 2024 15:27

kmilo9999 requested a review from cpaniaguam January 24, 2024 18:31

added h5py to depdency list

539c25f

cpaniaguam requested changes Jan 24, 2024

View reviewed changes

.github/workflows/test-B01_SL_load_single_file.yml Outdated Show resolved Hide resolved

kmilo9999 and others added 10 commits January 24, 2024 18:14

Apply suggestions from code review

78e56c6

Co-authored-by: Carlos Paniagua <[email protected]>

suggested changes in B04_angle.py

2f5f22c

Merge branch 'feat-step5-intg' of https://github.com/brown-ccv/icesat…

08feb9c

…2-tracks into feat-step5-intg

add nan_polocy to omit nans in fitting model function

f4d808e

patch angle_optimizer to avoid CI crash on step5 test

fe6db3e

something went wrong pushing this file in previous commit

6fed1f1

test patched file

326e218

test just one step in the workflow

9d5a703

test just one step in the workflow

a7f0067

testing workflow again

f1e737e

kmilo9999 requested a review from cpaniaguam January 25, 2024 16:18

kmilo9999 added 4 commits January 25, 2024 11:22

testing again with local change in optimize function

f52f46c

patching the file before process starts

a6cbfe2

rollback to only modify angle optimizer

400bec7

removing patch file and add comment in the line that makes CI crash

4621289

cpaniaguam requested changes Jan 26, 2024

View reviewed changes

kmilo9999 added 2 commits January 26, 2024 11:12

removing deprecated import and moving constanat values out of loops

8f88fd6

removing patch file

7bff883

kmilo9999 requested a review from cpaniaguam January 26, 2024 16:22

cpaniaguam requested changes Jan 26, 2024

View reviewed changes

kmilo9999 and others added 2 commits January 26, 2024 13:05

Apply suggestions from code review

9fc456a

Co-authored-by: Carlos Paniagua <[email protected]>

removing unsued variable from loop

ad61465

kmilo9999 requested a review from cpaniaguam January 26, 2024 18:12

cpaniaguam approved these changes Jan 27, 2024

View reviewed changes

kmilo9999 merged commit 068717d into main Jan 29, 2024
1 check passed

kmilo9999 deleted the feat-step5-intg branch January 29, 2024 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature step5 integration #51

Feature step5 integration #51

kmilo9999 commented Dec 28, 2023

cpaniaguam Jan 12, 2024

cpaniaguam Jan 12, 2024

kmilo9999 Jan 12, 2024

cpaniaguam left a comment

cpaniaguam left a comment

cpaniaguam Jan 26, 2024

kmilo9999 Jan 26, 2024 •

edited

Loading

cpaniaguam Jan 26, 2024

kmilo9999 Jan 26, 2024

cpaniaguam Jan 26, 2024

kmilo9999 Jan 26, 2024

cpaniaguam Jan 26, 2024

kmilo9999 Jan 26, 2024

cpaniaguam Jan 26, 2024

cpaniaguam Jan 26, 2024

kmilo9999 Jan 26, 2024

cpaniaguam Jan 26, 2024

kmilo9999 Jan 26, 2024

cpaniaguam left a comment

cpaniaguam Jan 26, 2024

cpaniaguam left a comment

	from scipy.ndimage.measurements import label
	from scipy.ndimage import label

Feature step5 integration #51

Feature step5 integration #51

Conversation

kmilo9999 commented Dec 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpaniaguam left a comment

Choose a reason for hiding this comment

cpaniaguam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmilo9999 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpaniaguam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpaniaguam left a comment

Choose a reason for hiding this comment

kmilo9999 Jan 26, 2024 •

edited

Loading