Skip to content
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

Morphology recentering for SONATA circuits. #229

Merged
merged 2 commits into from
Jun 11, 2018
Merged

Morphology recentering for SONATA circuits. #229

merged 2 commits into from
Jun 11, 2018

Conversation

hernando
Copy link
Contributor

@hernando hernando commented Jun 7, 2018

Added Circuit::getAttribute to retrieve custom node attributes
from SONATA circuits.

Added Circuit::getAttribute to retrieve custom node attributes
from SONATA circuits.
Copy link
Contributor

@karjonas karjonas left a comment

Choose a reason for hiding this comment

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

Looks fine. Is it possible to unit test this?

size_t numRecentered = 0;
size_t numOriginal = 0;
};
using Loading = std::unordered_map<std::string, MorphologyUse>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use a type alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the beginning I thought I was going to use this type more than once.

@hernando
Copy link
Contributor Author

hernando commented Jun 8, 2018

I'm trying to provide a test (despite it's quite annoying), but I won't be able to test all combinations. To me this is the type of code that is better to review thoroughly rather than rely on tests and coverage. The tests need to be white box to guarantee coverage and given the amount of combinations 100% coverage doesn't guarantee very much.

@hernando hernando force-pushed the recentering branch 4 times, most recently from 5d10241 to 7883dfe Compare June 8, 2018 13:11
@hernando
Copy link
Contributor Author

hernando commented Jun 8, 2018

Done. I also fixed a couple of bugs I found out while doing it and testing with the sample circuit I have

The dataset names used for rotation angles had a typo.
The indexing used to access node properties from the model types CSV
was wrong (it was mixing gids and node types ids).

Fixed typo in rotation angle attribute names.
@hernando
Copy link
Contributor Author

hernando commented Jun 8, 2018

Retest this please

auto transforms = circuit.getTransforms({0, 2});
morphologies =
circuit.loadMorphologies({0, 2}, brain::Circuit::Coordinates::global);
BOOST_CHECK_EQUAL(morphologies[0]->getSoma().getCentroid(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all transforms identity in the test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, how do you reach that conclusion?

@hernando hernando merged commit 0c68955 into master Jun 11, 2018
@hernando hernando deleted the recentering branch June 18, 2018 10:27
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.

None yet

2 participants