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

new script to generate edge topology #272

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kanonet
Copy link
Contributor

@kanonet kanonet commented Jul 22, 2019

This is a script to generate the edge_topology.json in python only, without matlab. Generated json does contain the same data like the ones generated with old process, just sorted differently. I think sorting order does not matter in this case, so results are compatible and this script can completely replace the old ones.
Please review. Also please elaborate how to give credits to the autors of the original scripts since this is still based on their general idea.

@patrikhuber
Copy link
Owner

Hi Christoph,

Thanks a lot for the PR, that's a great idea and definitely simplifies the process. I'd like to check the code in a bit more details myself, it'll probably be a while, until around end of August - I'll be travelling/away. But it looks pretty much good to go 👍

From the top of my head, I'd agree that the sorting should not make a difference.

please elaborate how to give credits to the autors of the original scripts

I think if you copy the top of the header from compute_edgestruct.m to the new Python script, that should be completely fine.

Also it would be great if you could convert the tabs to spaces and perhaps run a PEP 8 formatting (though the rest looks pretty consistent). All major Python IDEs should have such an option (I'm using PyCharm) and there are command-line tools for it as well.

Thanks again!

@kanonet
Copy link
Contributor Author

kanonet commented Jul 22, 2019

Hi Patrik,

I cleaned up the code, it should be more readable now. Its PEP8 compliant now (even though I think 79 chars per line are to few...).

Since its all packed in a function, you can call this from your converter scripts to simplify the process even more.

Thanks for creating this project :)

P.S.: I will probably publish some additions to the python binding in next days. Do you want one PR for each function or one PR for every single commit?

@patrikhuber
Copy link
Owner

Hi Christoph,

Just wanted to let you know that I still have your PR(s) on my todo list. Things are quite busy currently but I should get to it within the next two of weeks. Apologies that it's taking a while.

@kanonet
Copy link
Contributor Author

kanonet commented Nov 1, 2019

Hi @patrikhuber
dont want to bother you to much, I just want to know, if you are still working on checking this? Also please not my other PR and those pybind additions that I do in my fork (and would like to PR here, if you want). Best Regards.

@patrikhuber
Copy link
Owner

patrikhuber commented Nov 5, 2019

Hi Christoph,
You're not bothering at all. I apologise that this is taking so long. I have it on my to do list but haven't managed to get to it yet, it has been incredibly busy :/. Please bare with me for a bit longer, I am definitely not forgetting about it.

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.

2 participants