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

Roll calculation #185

Merged
merged 22 commits into from
Jul 8, 2024
Merged

Roll calculation #185

merged 22 commits into from
Jul 8, 2024

Conversation

TakKanekoGit
Copy link
Contributor

@TakKanekoGit TakKanekoGit commented Jul 7, 2024

First try at resolving the roll issue. During IMU dead-reckoning, integrator.py calls calc_utils.sf_utils.radec_to_roll() to calculate the expected roll at (RA, Dec) give the observer's location and time. The roll value is saved to solved["Roll"].

The functions/methods have been unit tested but not system tested in a PiFinder. It also needs to be field tested to check it fixes the roll issue.

@brickbots
Copy link
Owner

Thank you @TakKanekoGit for all this great work and for including great tests along with it! I think this is ready to merge and test under the stars 👍

@brickbots brickbots merged commit 85f0885 into brickbots:main Jul 8, 2024
1 check failed
@brickbots
Copy link
Owner

Just for reference here, because it's not documented in the dev guide 😅 , I've run the code formatter Black on the squash commit/code. You can check out the changes Black made in the subsequent commit from me to Main.

If you have installed the requirements_dev.txt requirements, the proper version of Black is in there and you should just need to run Black . from the python directory to check all the code from that folder down. It generally just reformats things but will rarely indicate some issue that needs to be resolved by hand.

Sorry about the lack of documentation for this, in the next few weeks we'll be moving to a more robust linting/formating/static type checking system that is documented in the dev guide... just working on finalizing that release as it's got a ton of code changes 👍

@TakKanekoGit TakKanekoGit deleted the roll_calculation branch July 8, 2024 19:21
@TakKanekoGit
Copy link
Contributor Author

Thank you for sorting out the linting and the tip about Black. I'll check it before I contribute code next time. Out of interest, do you usually run it at the end or use a Black extension in your editor?

Hope it works under the skies. Fingers crossed.

@brickbots
Copy link
Owner

My pleasure! I'm very excited to test this out and I appreciate the great comments and overall code quality 💯

I use Black as a pre-commit hook as I develop a fair bit on the actual PiFinder via SSH/tmux/Vim, but I am pretty sure it can be integrated in various editors.

In the very near future we'll be moving to a more robust set of tools orchestrated through nox:
Ruff - Faster linter/formatter which implements the same basic rules as Black
MyPy - Static type analysis, we have a branch with appropriate code changes, rules/ignores so that the current code base passes
PyTest - Switching from unittest to PyTest. I've already ported your great calc_utils test over to PyTest in this branch

So if you are going to be setting up a dev environment, this would probably be a better target as in the next few weeks the project will switch over 👍

@TakKanekoGit
Copy link
Contributor Author

Thank you for the heads on the upcoming new tools. Very interested to try them out when the switch over happens.

I'll work on step 2 of the roll issue fix (taking into account the offset) after the summer break and test it on the PiFinder when all the bits arrive. As part of step 2, I will also try to fix issue #117. I'll add the proposed solution direction in the issue to check with you.

#117

Thanks for your help!

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