Scitools Understand Parser #309
Conversation
Modeled after parse_dependencies with a similar output, this is for review to see if it is in-line with specification
Per the specifications in Issue 308, the files are functional and ready to be put into a notebook.
Quick fix
Updating NAMESPACE to export the new functions and creating the Rmd, are the primary notes. The folder holding the sample project also is included locally, but uses the calculator project provided in Issue from Carlos
|
@RavenMarQ just a reminder the notebook should have more than just code, please check with @daomcgill or ask her for a code review if needed. |
There was a problem hiding this comment.
I am a bi concerned with the use of loops here and the overall code logic, did you follow
https://github.com/sailuh/kaiaulu/blob/c7811069ef3c7943b509c76d4b9116d1a14dc343/R/src.R#L373C1-L378
?
|
In addition to the above, see CONTRIBUTING.md on how to edit NEWS.md, and DESCRIPTION. A lot of files that shouldn't be added have been pushed in this PR and those that should be edited were not. |
Most of the code review notes have been resolved, except for changing the file.path and descriptions. The notebook is currently being updated, but a preview on the proposed format is provided.
There was a problem hiding this comment.
The reason for having a notebook is to help users understand what they can do with it, and why they would want to use the features in the first place (as well as understanding how to use the code). The user should not need to read through your issue or code in order to know what is going on. This is the place to explain it to them.
Having addressing most things, especially using file.path, the functions are functional and the notebook works.
Resolving comments received on the notebook.
- Refactored the understand_showcase.Rmd notebook to expect the use of the getters from R/config.R (i #230 contains the getter functions in R/config.R).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #309 +/- ##
==========================================
- Coverage 39.79% 37.94% -1.85%
==========================================
Files 20 20
Lines 3091 3355 +264
==========================================
+ Hits 1230 1273 +43
- Misses 1861 2082 +221 ☔ View full report in Codecov by Sentry. |
- The project configuration sections of a notebook was incorrectly using the project directory (kaiaulu/) as its working directory rather than the directory that it resides in (/vignettes/) as its working directory.
|
@daomcgill the GitHub Actions problem was incidentally isolated: The moment I merged The GitHub Refresher, master started passing. I thought GitHub Actions broke something, but the problem is deeper and in our end. Notice how @RavenMarQ PR also passed just now after I merged the passing master. @beydlern @crepesAlot please merge master to your PRs that are still open. master is currently passing, as so is this PR. If yours is not passing, I can't merge. Ping me on your PRs that do not pass master so we can diagnose. If you had to merge Dao's PR, then the issue on her code likely spread to yours. |
|
I will be looking at this code next, please do not commit to it until I confirm otherwise. |
Signed-off-by: Carlos Paradis <carlosviansi@gmail.com>
|
@daomcgill @RavenMarQ @crepesAlot @beydlern please make sure you are not adding unrelated files using git add . on everything on the other PRs you made, see 222e3ba. |
|
@RavenMarQ The example on this notebook is Kaiaulu on the config, but the specified language to understand is |
|
The notebook does not call tools.yml, yet scitools is a third party tool. It should not assume every computer has "und" readily available. |
Changed conf used to helix, the same used as depends_showcase notebook Added scitools understand to tools.yml as not every computer will have und available. Signed-off-by: Carlos Paradis <carlosviansi@gmail.com>
This modifies the XML files so they are saved locally. This adds an additional function step, removes the scitools and dependency from parser. The independent parser function means the raw data is not locked behind the db, should a new version make it incompatible to extract in the future. It also facilitates inspection of the raw data should the parser ever break in the future. Signed-off-by: Carlos Paradis <carlosviansi@gmail.com>
|
@daomcgill @RavenMarQ @crepesAlot @beydlern consider commit message a38e617 for any downloaders or parsers you may make in the future, it is a variant case of "keeping raw data locally". |
|
Current implementation of transformer assumes no duplicate ever happens. This is fine in general for a true network, but using the "label" to name nodes causes the function to create the visualization to throw errors due to duplicates. For example, the file With a full path this can be avoidable, but the graph becomes unreadable. The most sane way to proceed here is to concatenate the id that scitools gives to the node to the label, therefore making it unique. However, the same correction needs to be done to the edges. Since this is something needed for graph only, the logic will be placed on the transform function. |
Concatenate labels for node and edges Signed-off-by: Carlos Paradis <carlosviansi@gmail.com>
Passes unit tests, notebooks, etc. Signed-off-by: Carlos Paradis <carlosviansi@gmail.com>
This is to test Committing and PR