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

Add more monitors in ops #430

Merged
merged 7 commits into from
Sep 27, 2017
Merged

Add more monitors in ops #430

merged 7 commits into from
Sep 27, 2017

Conversation

JanisGailis
Copy link
Member

What the title says.

@JanisGailis
Copy link
Member Author

@mzuehlke You're the expert on observing monitors! Can you please check this out? I felt a bit unsure in places if what I'm doing is the correct thing to do. Especially in correlation (monitor.observing encompasses a lot of code) and in index.py and timeseries.py.

@JanisGailis JanisGailis self-assigned this Sep 26, 2017
@JanisGailis JanisGailis added this to the 1.0 milestone Sep 26, 2017
@mzuehlke
Copy link
Collaborator

I will take a look

@codecov-io
Copy link

codecov-io commented Sep 26, 2017

Codecov Report

Merging #430 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   75.16%   75.22%   +0.06%     
==========================================
  Files          72       72              
  Lines       10268    10293      +25     
==========================================
+ Hits         7718     7743      +25     
  Misses       2550     2550
Impacted Files Coverage Δ
cate/ops/index.py 100% <100%> (ø) ⬆️
cate/ops/anomaly.py 100% <100%> (ø) ⬆️
cate/ops/timeseries.py 100% <100%> (ø) ⬆️
cate/ops/arithmetics.py 98.27% <100%> (+0.16%) ⬆️
cate/ops/correlation.py 98.91% <100%> (+0.16%) ⬆️

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 1830f5d...3b01860. Read the comment docs.

@mzuehlke
Copy link
Collaborator

mzuehlke commented Sep 27, 2017

I only looked at the operators you had modified and improved the code in some places.

_pearsonr I looked a bit more closely by making sure the input in one test is chunked and the then looking at the output of the RecordingMonitor and by locally enabling _DEBUG_DASK_PROGRESS in "monitor.py".

Copy link
Collaborator

@mzuehlke mzuehlke left a comment

Choose a reason for hiding this comment

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

Feel free to merge

r.values[r.values < -1.0] = -1.0
r.values[r.values > 1.0] = 1.0
with monitor.child(1).observing("task 1"):
negativ_r = r.values < -1.0
Copy link
Member Author

@JanisGailis JanisGailis Sep 27, 2017

Choose a reason for hiding this comment

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

@mzuehlke
So, we only monitor when we try to access the values? As in, processing from lines 239-247 would be deferred until then?
EDIT: Oh my god I'm dumb, I just read my own comment.

@JanisGailis JanisGailis merged commit c1155d6 into master Sep 27, 2017
@mzuehlke
Copy link
Collaborator

I will in a calm moment try to make the dask progress monitor a bit smarter. Because I find this multiple observing calls not the nicest solution.....

@JanisGailis
Copy link
Member Author

So, for future reference - as it is now, we can only wrap a single line in with monitor.observing(): right?

@JanisGailis JanisGailis deleted the xx-jg-more-monitors branch September 27, 2017 09:42
@mzuehlke
Copy link
Collaborator

Currently: yes

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.

3 participants