Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Move earth sensor computations to core #1191

Merged
merged 1 commit into from Jun 11, 2021

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented May 1, 2021

This PR tries to move computations related to earth sensors to core.

Additionally, the conversion of variable names (in the extended ascii format) to printable ascii characters have been made for consistency.

Thanks!!

@codecov
Copy link

codecov bot commented May 1, 2021

Codecov Report

Merging #1191 (7acc3cf) into main (b37e5fe) will increase coverage by 0.04%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1191      +/-   ##
==========================================
+ Coverage   91.23%   91.27%   +0.04%     
==========================================
  Files          78       79       +1     
  Lines        4106     4127      +21     
  Branches      360      360              
==========================================
+ Hits         3746     3767      +21     
  Misses        270      270              
  Partials       90       90              
Impacted Files Coverage Δ
src/poliastro/core/sensors.py 91.17% <91.17%> (ø)
src/poliastro/earth/sensors.py 100.00% <100.00%> (+8.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b37e5fe...7acc3cf. Read the comment docs.

@Yash-10 Yash-10 force-pushed the Earth-sensors branch 2 times, most recently from 44c2983 to f6ffc3c Compare May 2, 2021 05:33
@Yash-10
Copy link
Member Author

Yash-10 commented May 3, 2021

@astrojuanlu I think #1196 could get resolved here. Could you have a look?

The codecov/patch test didn't execute for some reason. I am trying to figure out the reason for this..

Thanks!

@astrojuanlu
Copy link
Member

Indeed, #1196 got solved here! Do you know if it's just the whitespace or something else?

@Yash-10
Copy link
Member Author

Yash-10 commented May 3, 2021

Do you know if it's just the whitespace or something else?

I think it would not have to do with whitespace issues. I would try to find the reason behind this :)

@astrojuanlu
Copy link
Member

I opened #1199 to discuss about variable names. But I'm +0 (meaning: not totally convinced, but slightly positive) on this change.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Left some comments, but apart from that, this looks good to me!

This is on the verge of not being merged though, since I am preparing the beta release now. So if we could make a smaller PR just fixing the docstring issues, that would be great.

src/poliastro/earth/sensors.py Outdated Show resolved Hide resolved
Returns
-------
Λ_min: ~astropy.units.Quantity
lambda_min: ~astropy.units.Quantity
Copy link
Member

Choose a reason for hiding this comment

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

For some reason, ~astropy.units.Quantity does not always work:

Screenshot_2021-05-03 poliastro earth sensors — poliastro 0 15 dev0 documentation(1)

src/poliastro/earth/sensors.py Outdated Show resolved Hide resolved
tests/tests_earth/test_sensors.py Show resolved Hide resolved
@Yash-10 Yash-10 mentioned this pull request May 3, 2021
@Yash-10
Copy link
Member Author

Yash-10 commented May 3, 2021

This is on the verge of not being merged though, since I am preparing the beta release now.

Thanks for the suggestions, I would make the required changes and we shall look at it after the release!

@Yash-10 Yash-10 force-pushed the Earth-sensors branch 2 times, most recently from 0b59893 to 3914035 Compare May 3, 2021 16:51
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks again for this @Yash-10 ! I have recently renamed one of the functions, could you please rebase this on top of current main?

@Yash-10
Copy link
Member Author

Yash-10 commented May 16, 2021

Sorry @astrojuanlu for not keeping this branch up to date. While I rebase, could you let me know from the discussions in #1199, whether we have decided to keep the variable names as they are in current main or the one I had changed?

Thanks!

@Yash-10 Yash-10 force-pushed the Earth-sensors branch 6 times, most recently from e590710 to feffbcb Compare May 24, 2021 05:55
@Yash-10
Copy link
Member Author

Yash-10 commented May 24, 2021

Since it was decided to use ascii variable names at the high-level and allow using non-ascii at the low level, I have tried to change it accordingly.

The codeclimate failure seems a bit tricky. It says ground_range_diff_at_azimuth has 10 arguments but infact it has only 7 arguments. I wonder if it has something to do with the function being both jitted and uses non-ascii variables?

@Yash-10 Yash-10 force-pushed the Earth-sensors branch 2 times, most recently from d465b2e to cb30cb2 Compare May 25, 2021 05:35
@astrojuanlu
Copy link
Member

@Yash-10 sorry, don't pay much attention to CodeClimate, we are not strict about it (in fact, we look at it very rarely). You can revert your last commit it you want, I'll look at your PR soon.

@Yash-10
Copy link
Member Author

Yash-10 commented May 25, 2021

Thanks @astrojuanlu I reverted the recent commit.

It was interesting to note that changing variable names to ascii in core had removed the codeclimate issues. If there is a need, we could probably think of configuring codeclimate.yml to ignore such issues if non-ascii variables are used in core anywhere..

The codecov/patch status also seems to hang...

@astrojuanlu
Copy link
Member

@Yash-10 could you please rebase this one on top of main after we merge #1191? :)

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

This is good to go!

Minor changes for tests to pass

Change docstring in core

Small changes

Reformat code
@astrojuanlu astrojuanlu merged commit 1a25403 into poliastro:main Jun 11, 2021
@astrojuanlu
Copy link
Member

Automerge worked! Nice 😎

@Yash-10
Copy link
Member Author

Yash-10 commented Jun 11, 2021

Its really great! Thanks for merging!

@Yash-10 Yash-10 deleted the Earth-sensors branch June 11, 2021 10:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants