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

module submission: 2024-Jarigsma-001 #35

Merged
merged 69 commits into from
Dec 2, 2024

Conversation

AmberHam
Copy link
Contributor

No description provided.

AmberHam and others added 6 commits November 27, 2024 16:13
Co-authored-by: Andreas Angourakis <[email protected]>
Non-compulsory fields that are empty should be deleted. Alternatively, to keep them as placeholders, they could have a null value with `null` or `~`.  Making a note of this in the guide.
correcting indenting of line 12
adding compulsory field `lastUpdateDate`
docsDir and other directory or file references present in template should be deleted if not present in the current module. Making note of this in guide.
adding missing comas separating BibTex fields
@Andros-Spica Andros-Spica requested a review from nevrome November 28, 2024 11:58
start the module with v1.0.0 and mention it in CHANGELOG
Recommendations:
- Code style:
  - adding a few extra lines of comment for pedagogic purposes
  - `create-edge` -> `create-preferential-attachment`, to be more specific and consistent with the algorithm terminology.
  - `find-preferential-partner` -> `preferential-partner,` because while verbs are good for procedures, this reporter procedure will return a partner directly, instead, for example, of setting it as a variable internally. 
- Refactoring:
  - For improving re-usability, delimiting the preferential attachment algorithm as a node-level procedure `create-preferential-attachment` (partner finding + creating link + increasing degree) instead of the global-level procedure `create-edge`. 
  - For optimising, skipping unnecessary separation of `make-node` and `create-edge` (and the intermediary variable `new-node`) by using `create-preferential-attachment` directly inside `create-node 1 [ ... ]` (line 34) and using `other node` in `` (line 60) to ensure that the new node is not accounted as a potential partner of itself.
`number-of-nodes` -> `initial-number-nodes`
Copy link
Member

@Andros-Spica Andros-Spica left a comment

Choose a reason for hiding this comment

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

Hi Amber,

I took the liberty of correcting the issues in NASSA.yml that were causing problems with the automatic checks. Have a look at my commits.

As the actual review, I made a few suggestions as comments and again took the liberty of directly committing edits on the .nlogo file, just to make it more re-usable. I did it because the changes were conceptually simple, but a bit hard to add only as suggestions and comments. Please let me know if you don't agree with the changes and we discard them.
I have also added Clemens Schmid as reviewer (hopefully he agrees with my suggestions!).

Anyway, if you agree with the changes I already commited, you can focus on the comments and suggestions (all are not critical issues).

Best,
Andreas

2024-Jarigsma-001/README.md Show resolved Hide resolved
2024-Jarigsma-001/README.md Show resolved Hide resolved
2024-Jarigsma-001/README.md Outdated Show resolved Hide resolved
2024-Jarigsma-001/references.bib Outdated Show resolved Hide resolved
2024-Jarigsma-001/NASSA.yml Show resolved Hide resolved
@AmberHam
Copy link
Contributor Author

AmberHam commented Dec 1, 2024

Hi Andros,

I have taken a look at the changes you committed directly to the .nlogo file and I do agree that they make the code more reusable. I have also made a small change to that file but only in one of the comments by the nodes-own declaration. Other than that, I have updated the readme file according to your suggestions. I hope I have managed to address all the comments and suggestions but if I have missed anything then please let me know. Otherwise, if there are further changes that should be made, let me know also.

Best regards,
Amber

Adding Barabási's references to .bib file
Copy link
Member

@Andros-Spica Andros-Spica left a comment

Choose a reason for hiding this comment

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

Hi Ambar,
Everything looks good to me 👍
I just added Barabási's references to the references.bib file to make it more complete.
I will approve and we can finally merge it!

@Andros-Spica Andros-Spica merged commit eada722 into Archaeology-ABM:main Dec 2, 2024
1 check passed
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