Skip to content

Conversation

@gmegh
Copy link
Collaborator

@gmegh gmegh commented Mar 27, 2025

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@gmegh gmegh requested a review from jbkalmbach March 27, 2025 21:20
@gmegh gmegh force-pushed the tickets/SITCOM-1870 branch from e82e8e0 to 3c5c51f Compare March 27, 2025 21:58
@@ -0,0 +1,222 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

Line #8.        "day": {

Is this day_obs in butler terms? I think you should just name this day_obs and the next one to seq_num to make clear what they are asking for. Would also rephrase the description to "day and year". Or do you actually mean the "n'th" day of the year? I would suggest not doing that and using day_obs instead.


Reply via ReviewNB

@@ -0,0 +1,222 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

Line #23.        "maxiter": {

I Just noticed that we have out configurations all using snake case except this one in all the scripts. Maybe we should change to be consistent?


Reply via ReviewNB

@@ -0,0 +1,222 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

Line #1.    ranges = [150, 5000, 5000, 0.1, 0.1]

These ranges and test patterns should be added somewhere in Zephyr scale for reference.


Reply via ReviewNB

@@ -0,0 +1,222 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

Line #8.            name="maintel/set_dof.py",

Setting up set_dof.py like this requires a rewrite of set_dof.py right? Is that work recorded as a TODO somewhere and linked to this?


Reply via ReviewNB

@@ -0,0 +1,222 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

I think all the same comments as the notebook above.


Reply via ReviewNB

@@ -0,0 +1,221 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

Same comments as T413 notebook


Reply via ReviewNB

@@ -0,0 +1,232 @@
{
Copy link
Member

@jbkalmbach jbkalmbach Mar 31, 2025

Choose a reason for hiding this comment

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

Same comments as T413 notebook


Reply via ReviewNB

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