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

optimizing orbit.sample #379

Merged
merged 4 commits into from May 23, 2018

Conversation

2 participants
@nikita-astronaut
Copy link
Contributor

nikita-astronaut commented May 21, 2018

Hello, @Juanlu001

Did I understand correctly how you proposed it to be? Perhaps the current version seems quite ugly, though uses dense_output.

The problem is that cowell function can be called from propagate and from sample methods, that's why we need at some places first wrap into [] and then unwrap [0].

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 22, 2018

Codecov Report

Merging #379 into master will increase coverage by 0.38%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   82.75%   83.14%   +0.38%     
==========================================
  Files          33       33              
  Lines        1722     1726       +4     
  Branches      137      141       +4     
==========================================
+ Hits         1425     1435      +10     
+ Misses        258      252       -6     
  Partials       39       39
Impacted Files Coverage Δ
src/poliastro/twobody/orbit.py 95.62% <100%> (+0.09%) ⬆️
src/poliastro/twobody/propagation.py 91.83% <93.33%> (+6.26%) ⬆️

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 b833657...64e5c1a. Read the comment docs.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented May 22, 2018

Did I understand correctly how you proposed it to be? Perhaps the current version seems quite ugly, though uses dense_output.

You did it exactly the way I thought of it, and it's uglier than I thought :)

To justify the ugliness, can you do a simple %timeit with both versions to see how faster is the dense output one? If it's a really noticeable performance increase, let's get this merged and I will propose an alternative object design.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

I made one comment about the code. It will also fix one of the codeclimate issues, I think.

rrs.append(y[:3])
vvs.append(y[3:])

if len(tof) == 1:

This comment has been minimized.

@Juanlu001

Juanlu001 May 22, 2018

Member

If the user propagates with a list of times, I would always expect a list, even if the input list has only one element. So I would not make a special case here.

This comment has been minimized.

@nikita-astronaut

nikita-astronaut May 23, 2018

Author Contributor

But we have to! The problem is, there are functions that propagate cowell not with the list, but with single time value (for instance come tests). And the output is expected to be, once again, single orbit. So, we need to track whether user passed list or single value to cowell and act accordingly.

What I wrote above is not exactly this, I admit: if user gives list, we should return list. But we have to, anyway, work with the special case when user passed just one single value not in the list.

This comment has been minimized.

@Juanlu001

Juanlu001 May 23, 2018

Member

I know, and that's why we have to distinguish between scalar value and list. But still you added a third case, which is converting a list of one element to the scalar case. As you said, if the user gives a list we should return a list.

# if use cowell, propagate to max_time and use other values as intermediate (dense output)
if method == cowell:
values, _ = cowell(self, (time_values - self.epoch).to(u.s).value)
if len(time_values) == 1:

This comment has been minimized.

@Juanlu001

Juanlu001 May 22, 2018

Member

See above

nikita-astronaut added some commits May 22, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented May 23, 2018

With your permission @nikita-astronaut, I'm pasting here your screenshot :) This change makes cowell sampling an order of magnitude faster. Until we have proper benchmarks (#376), this is more than enough to merge, even if the code is not extremely beautiful.

image

Merging!

@Juanlu001 Juanlu001 merged commit b659a94 into poliastro:master May 23, 2018

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate Approved by Juan Luis Cano Rodríguez.
Details
codecov/patch 95.65% of diff hit (target 82.75%)
Details
codecov/project 83.14% (+0.38%) compared to b833657
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nikita-astronaut nikita-astronaut deleted the nikita-astronaut:new_Cowell_API branch May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment