Skip to content

Conversation

hkethi002
Copy link
Contributor

Fixes #915

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@hkethi002 hkethi002 requested a review from nagem September 18, 2017 16:26
@codecov-io
Copy link

codecov-io commented Sep 18, 2017

Codecov Report

Merging #930 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #930      +/-   ##
=========================================
+ Coverage   90.19%   90.2%   +<.01%     
=========================================
  Files          48      48              
  Lines        6418    6410       -8     
=========================================
- Hits         5789    5782       -7     
+ Misses        629     628       -1
Flag Coverage Δ
#python 90.2% <100%> (ø) ⬆️
Impacted Files Coverage Δ
api/handlers/reporthandler.py 94.4% <100%> (+0.12%) ⬆️

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 124ea0c...1e63c37. Read the comment docs.


# Grab acquisitions and their ids
acquisitions = config.db.acquisitions.find({'session': {'$in': session_ids}}, {'_id': 1, 'analyses':1})
acquisition_ids = [a['_id'] for a in acquisitions]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the projection for analyses above.


ids = session_ids + acquisition_ids + [p['_id']]
analysis_ids = [an['_id'] for an in config.db.analyses.find({'parent.id': {'$in': ids}})]

Copy link
Contributor

Choose a reason for hiding this comment

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

Below there is more logic that would have to change now that analyses are in a separate collection.

@nagem
Copy link
Contributor

nagem commented Sep 18, 2017

Nice catch!

I also noticed there is a problem with how we are calculating analysis files - we would only want to calculated "output" files. There is a way to conditionally project in an aggregation pipeline so you can set the size of input files as 0 and output files as expected before the sum step to fix this problem.

@nagem
Copy link
Contributor

nagem commented Oct 11, 2017

Final changes look good, feel free to merge 👍

@hkethi002 hkethi002 merged commit 226f425 into master Oct 11, 2017
@hkethi002 hkethi002 deleted the usage-500 branch October 11, 2017 21:39
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.

3 participants