Skip to content

Conversation

@arnaudon
Copy link

@arnaudon arnaudon commented Jun 25, 2020

New parameter use_apical_points in the config dict to disable the use of apical_points.
Small bug from previous PR on allowing for yaml recipe file, the layer info should be a str so that dataframe.merge can add the etypes to the dataframes.

@arnaudon arnaudon requested a review from anilbey June 25, 2020 10:59
@arnaudon
Copy link
Author

can we increase the lengths of line to at least 100?
this is a pain:
bluepymm/prepare_combos/parse_files.py:133:80: E501 line too long (80 > 79 characters)

@arnaudon arnaudon requested a review from wvangeit June 25, 2020 11:17
)
try:
apical_points_isec = tools.load_json(
os.path.join(one_row['morph_dir'], "apical_points_isec.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need the apical for MM?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the idea is that for the current release we don't use the bAP feature for the selection, so we don't need the values.
But I consider this fix fairly dangerous. I'd propose we add an option to the config file to actively disable the apical points

@arnaudon
Copy link
Author

arnaudon commented Jun 25, 2020 via email

)
try:
apical_points_isec = tools.load_json(
os.path.join(one_row['morph_dir'], "apical_points_isec.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the idea is that for the current release we don't use the bAP feature for the selection, so we don't need the values.
But I consider this fix fairly dangerous. I'd propose we add an option to the config file to actively disable the apical points

@arnaudon
Copy link
Author

Ok, I'll add the option in the config file!

@arnaudon
Copy link
Author

it is not so obvious, the dict is not used in the run step, only in the prepare I think

@arnaudon arnaudon changed the title no apical + str for yaml + no repo pool new option for no apical + str for yaml Jun 25, 2020
@arnaudon
Copy link
Author

There is a weird bug I can't quite understand how to fix, @anilbey, could you have a look, thanks!

@anilbey
Copy link
Contributor

anilbey commented Jun 25, 2020

There is a weird bug I can't quite understand how to fix, @anilbey, could you have a look, thanks!

Do you mean the sqlite error below that's making travis fail?

sqlite3.OperationalError: near ")": syntax error

@wvangeit
Copy link
Contributor

Yeah, I think so

@arnaudon
Copy link
Author

yes!

@anilbey
Copy link
Contributor

anilbey commented Jun 25, 2020

Ok, I'm on it.

@anilbey
Copy link
Contributor

anilbey commented Jun 25, 2020

That's the error ValueError: Key mm.bpo_holding_current not found in responses. It's a familiar one. I think it is due to changes in calculate_scores.

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #200 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   72.06%   72.12%   +0.06%     
==========================================
  Files          21       21              
  Lines        1353     1356       +3     
==========================================
+ Hits          975      978       +3     
  Misses        378      378              
Flag Coverage Δ
#functional 72.12% <100.00%> (+0.06%) ⬆️
#unit 60.98% <66.66%> (-0.14%) ⬇️
Impacted Files Coverage Δ
bluepymm/prepare_combos/prepare_emodel_dirs.py 95.00% <ø> (ø)
bluepymm/prepare_combos/parse_files.py 97.95% <100.00%> (ø)
bluepymm/run_combos/calculate_scores.py 90.83% <100.00%> (ø)
bluepymm/run_combos/main.py 100.00% <100.00%> (ø)

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 3b20798...02136a8. Read the comment docs.

@wvangeit wvangeit merged commit b0e1d14 into master Jun 26, 2020
@wvangeit wvangeit deleted the apical_and_yaml branch June 26, 2020 10:10
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.

5 participants