Skip to content

Conversation

@edoardolegnaro
Copy link
Contributor

No description provided.

Comment on lines 26 to 28
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to not have this hard coded

Copy link
Contributor

Choose a reason for hiding this comment

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

read from config - or pass as variable

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use units or failing that np.radtodeg or np.degtorad

Comment on lines 21 to 31
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@samaloney
Copy link
Contributor

Eh looks much better the example code is evaluated so it has to run which is why the test are failing. The examples are great but don't need them on every function if it not trivial to setup the variables etc

@samaloney samaloney mentioned this pull request Oct 14, 2024
Comment on lines 116 to 117
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary

Suggested change
if __name__ == "__main__":
pytest.main([__file__])

setup.cfg Outdated
Comment on lines 69 to 72
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a strategy here specifically if we change a major version 1.x.y to 2.x.y probably need rerun the dataset generation and training

Comment on lines -23 to -24
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a sunpy version update change so need to be careful with these

Copy link
Contributor

Choose a reason for hiding this comment

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

If these should be treated as constants would usually uppercase them?

@samaloney samaloney merged commit a630048 into ARCAFF:main Feb 10, 2025
8 checks passed
@edoardolegnaro edoardolegnaro deleted the PR/utils branch March 25, 2025 10:30
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.

2 participants