Skip to content

add -swc in pynml #326

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 9, 2024
Merged

add -swc in pynml #326

merged 5 commits into from
Apr 9, 2024

Conversation

@YBCS
Copy link
Contributor

@YBCS YBCS commented Mar 24, 2024

closes #133

  • adds a new cli arg in pynml to allow converting NeuroML to swc format

@YBCS
Copy link
Contributor Author

YBCS commented Mar 24, 2024

@sanjayankur31 PTAL

@sanjayankur31 sanjayankur31 self-requested a review March 25, 2024 23:48
@sanjayankur31
Copy link
Member

Looks good @YBCS . Let's add some tests etc. to make sure this is always checked:

  • can you please add a test command in test.sh where it says "test some conversions"? You can use the example files in examples, the ones that have been used in ExportSWC.py
  • it seems to print every line during the conversion, can you turn that off---for large conversions, that'll be a lot of output. Could you convert the print statement to a logger.debug perhaps?
  • could you look at generating the man pages again? you need to run the generate-man-pages.sh script, you'll need to have help2man installed
  • could you check the doc-string for the -swc option you've added? Can one provide multiple files? If yes, that doc string needs to reflect that.

sanjayankur31
sanjayankur31 requested changes Mar 26, 2024
View reviewed changes
Copy link
Member

@sanjayankur31 sanjayankur31 left a comment

Choose a reason for hiding this comment

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

Requesting changes

@sanjayankur31 sanjayankur31 linked an issue Mar 26, 2024 that may be closed by this pull request
Add -swc option to export NeuroML cell files to SWC format #133
Closed
@YBCS
Copy link
Contributor Author

YBCS commented Mar 27, 2024 •
edited
Loading

@sanjayankur31 I have addressed the comments please take a look !

I noticed that some nml files despite being valid does not have its corresponding swc representation, so I have added simple checks to count successful conversion

Also I have decided to use same file as was used before NML2_SingleCompHHCell.nml , for testing nml file conversion in test.sh so that I dont have to change the directory. I have tested the file and it works.

@YBCS YBCS requested a review from sanjayankur31 April 8, 2024 10:28
@sanjayankur31
Copy link
Member

I've just returned from leave, so I'll take a look this week.

sanjayankur31
sanjayankur31 requested changes Apr 9, 2024
View reviewed changes
YBCS added 2 commits April 9, 2024 22:18
@YBCS
Merge branch 'development' into dev-buda-development
1ba6112 
* development:
  chore(vispy): add note about env var
  fix(tests): only parse sims in parallel
  fix(tests): compile mod files before testing
  test(runners): add new test
  feat(runners): add general multi command runner wrapper
  fix: handle cases where args/kwargs are not provided
  chore: do not re-compile mods in examples folder
  chore(runners): correct rst in docstring
  chore(runners): correct documentation formatting
  chore(runners): correct documentation
  deps: make ppft a default dep
  feat(runners): add parallel runner and tests
@YBCS
run generate man in right dir, check cell length
2d4a7ea
@YBCS YBCS requested a review from sanjayankur31 April 9, 2024 17:25
@sanjayankur31
Copy link
Member

I think that looks good now. There are a few issues with the exporter, but I'll file fresh issues for them separately. Let's wait for CI to finish, and then I can merge this.

A few notes: we can write better git commit messages. For example "address comments" doesn't mean anything---in the future if you come across that commit, it won't tell you anything. Take a look at this post on writing good git commit messages:

https://cbea.ms/git-commit/

and this one on conventional commits---we don't enforce conventional commits, but they would be good to use since they really improve the readability of commits:

https://www.conventionalcommits.org/en/v1.0.0/

@sanjayankur31 sanjayankur31 merged commit f744cbb into NeuroML:development Apr 9, 2024
@sanjayankur31
Copy link
Member

@all-contributors please add @YBCS for code

@allcontributors allcontributors bot mentioned this pull request Apr 9, 2024
Merged
@allcontributors
Copy link
Contributor

@sanjayankur31

I've put up a pull request to add @YBCS! 🎉

@YBCS YBCS deleted the dev-buda-development branch April 10, 2024 02:55
@YBCS
Copy link
Contributor Author

YBCS commented Apr 10, 2024

I think that looks good now. There are a few issues with the exporter, but I'll file fresh issues for them separately. Let's wait for CI to finish, and then I can merge this.

A few notes: we can write better git commit messages. For example "address comments" doesn't mean anything---in the future if you come across that commit, it won't tell you anything. Take a look at this post on writing good git commit messages:

https://cbea.ms/git-commit/

and this one on conventional commits---we don't enforce conventional commits, but they would be good to use since they really improve the readability of commits:

https://www.conventionalcommits.org/en/v1.0.0/

@sanjayankur31 Thanks, I will make the commit message more clear next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanjayankur31 sanjayankur31 sanjayankur31 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

NeuroML backlog
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add -swc option to export NeuroML cell files to SWC format

2 participants

@YBCS @sanjayankur31