Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Add a function to get ids before dataframe#168

Merged
mgeplf merged 18 commits intomasterfrom
add_getids
Dec 15, 2021
Merged

Add a function to get ids before dataframe#168
mgeplf merged 18 commits intomasterfrom
add_getids

Conversation

@alkino
Copy link
Member

@alkino alkino commented Nov 25, 2021

Fix #149

NadirRoGue
NadirRoGue previously approved these changes Nov 26, 2021
Copy link
Contributor

@NadirRoGue NadirRoGue left a comment

Choose a reason for hiding this comment

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

Everything looks good

@sergiorg-hpc
Copy link
Contributor

sergiorg-hpc commented Nov 26, 2021

@mgeplf I asked @alkino to make some changes here in regards to the min,max and positions. Even though the code looks clear and simpler by duplicating the loop that goes through all the ids, this is a bit tricky, because some reports might have millions of cells and we would be wasting time by duplicating the loop and going through all the cells twice.

Thus, I suggested to try to keep the original design while still not affecting the performance of libsonata. A simple lambda parameter could be an option and, thus, the fun parameter that Nico added for illustration purposes.

@sergiorg-hpc sergiorg-hpc force-pushed the add_getids branch 3 times, most recently from 773fdd2 to 300dd68 Compare November 26, 2021 15:13
@mgeplf
Copy link
Contributor

mgeplf commented Dec 1, 2021

I would like to get this merged before I do a libsonata release, but I'm still puzzled by the comments; can you have a look @alkino?

@alkino
Copy link
Member Author

alkino commented Dec 1, 2021

updated

@mgeplf
Copy link
Contributor

mgeplf commented Dec 14, 2021

I changed the name of the function, and tried to make the docstrings more clear. @NadirRoGue or @alkino could you clean them up further; things like getIds don't really convey what IDs are being retrieved. In addition, the format of the return would be helpful for things like getNodeIdElementIdMapping

@NadirRoGue
Copy link
Contributor

I've added some extra doc for the return format of getNodeIdElementIdMapping, let me know if that's what you mean @mgeplf . For the rest, both the API and doc seems clear to me.

@mgeplf
Copy link
Contributor

mgeplf commented Dec 15, 2021

Ok, I'll take it from here, I guess. Thanks.

@mgeplf mgeplf merged commit fcc8483 into master Dec 15, 2021
@mgeplf mgeplf deleted the add_getids branch December 15, 2021 08:13
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.

Access report mapping without the need to load a frame

5 participants