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

Fix output of nest.spatial in Read the Docs #2527

Merged
merged 10 commits into from
Dec 2, 2022

Conversation

med-ayssar
Copy link
Contributor

Fixes #2513

@med-ayssar med-ayssar changed the title Define distance object in the module init file Fix nest.spatial in Readthedoc Nov 15, 2022
@med-ayssar med-ayssar marked this pull request as ready for review November 16, 2022 08:13
@jougs jougs requested a review from hakonsbm November 16, 2022 10:21
@jessica-mitchell jessica-mitchell added T: Bug Wrong statements in the code or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 16, 2022
@jessica-mitchell jessica-mitchell changed the title Fix nest.spatial in Readthedoc Fix output of nest.spatial in Read the Docs Nov 16, 2022
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@med-ayssar Thanks for looking into this, I just checked the output, and it looks like its all there 🎉

I think in a separate PR we can look at combining the two spatial sections of the API

@med-ayssar
Copy link
Contributor Author

@med-ayssar Thanks for looking into this, I just checked the output, and it looks like its all there tada

I think in a separate PR we can look at combining the two spatial sections of the API

What do you mean by combining here? I think that I am missing some context.

@jessica-mitchell
Copy link
Contributor

There is nest.spatial and nest.lib.spatial - and these are separate sections in Read the Docs API, for "spatially-distributed nodes" and "spatial distributions", which is a bit confusing.
I want to know if we can either move lib.spatial into spatial or rename it. And clarify any relevant differences in the docs or simply combine them if that makes sense.

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Choose a reason for hiding this comment

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

I have just a couple suggestions for the code.

pynest/nest/spatial/hl_api_spatial.py Outdated Show resolved Hide resolved
pynest/nest/spatial/hl_api_spatial.py Outdated Show resolved Hide resolved
pynest/nest/spatial/hl_api_spatial.py Outdated Show resolved Hide resolved
@terhorstd terhorstd requested a review from Helveg December 1, 2022 09:50
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

We should avoid that distance is created at import time, which we can do by moving this logic to nest.spatial

pynest/nest/spatial/hl_api_spatial.py Outdated Show resolved Hide resolved
pynest/nest/spatial/hl_api_spatial.py Outdated Show resolved Hide resolved
@Helveg
Copy link
Contributor

Helveg commented Dec 1, 2022

There is nest.spatial and nest.lib.spatial - and these are separate sections in Read the Docs API, for "spatially-distributed nodes" and "spatial distributions", which is a bit confusing. I want to know if we can either move lib.spatial into spatial or rename it. And clarify any relevant differences in the docs or simply combine them if that makes sense.

I'm also highly in favor of merging the 2 modules! It's confusing.

@Helveg Helveg self-requested a review December 1, 2022 15:08
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

Looks good. nest.spatial.distance works, import time code is avoided and the docs render and reference to distance just fine.

@Helveg Helveg requested a review from hakonsbm December 1, 2022 16:17
@jessica-mitchell jessica-mitchell merged commit ebdc604 into nest:master Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Missing spatial user documentation
4 participants