Add simulation capabilities #99
Conversation
|
Hayden, can we create a separate branch for this? I.e., separate from development? Given the specific functionality it is providing, I though "simulation" would make sense. |
|
Do you recommend I separate the simulation from the toolbox entirely? i.e. the simulation utilizes the analytics toolbox instead of changing some of the data type like Course |
|
My gut feeling is to add this capability directly into the toolbox, at least this "naive" implementation where we just use the pass rate of the course. I'll have more questions and insight after doing a full review, but if changes are needed to base data type(s) (i.e. Course) we would implement those changes in this package rather than extending or overriding the type(s) in another package. |
There was a problem hiding this comment.
Overall looks really solid. Pretty much all of the changes are related to organizing things or sticking to snake case. Great work @jiachengzhang1!
src/CurricularAnalytics.jl
Outdated
| include("Simulation/Simulation.jl") | ||
| include("Simulation/PassRate.jl") | ||
| include("Simulation/Enrollment.jl") | ||
| include("Simulation/Report.jl") |
There was a problem hiding this comment.
Would it make sense to wrap these into one include("Simulation/Simulation.jl")? I was envisioning that the Simulation file would define a Simulation module which would bring in the other files
There was a problem hiding this comment.
Add the includes in Simulation/Simulation.jl 179c411
For module, since like DegreePlanAnalytics.jl and others don't using module, so may be we shouldn't use module for the simulation for consistency?
There was a problem hiding this comment.
Good point. For consistency we should do it the way you outlined, but I'm not sure if there are any benefits to putting things into modules... Might be an opportunity to improve performance?
Gonna include some links to come back to, but feel free to check them out. Once we merge this it might be worth checking out a branch to modularize everything.
https://docs.julialang.org/en/v1/manual/modules/
https://discourse.julialang.org/t/large-programs-structuring-modules-include-such-that-to-increase-performance-and-readability/29102
https://discourse.julialang.org/t/loading-only-what-you-need/39465
|
I agree, the capabilities should be added directly to the toolbox. |
|
Jiacheng,
I cloned your repo a few days ago, and have made some edits. What’s the best way to integrate these changes at this point?
Thanks,
-Greg
… On Aug 9, 2020, at 8:54 PM, Jiacheng Zhang ***@***.***> wrote:
@jiachengzhang1 commented on this pull request.
In src/Simulation/Enrollment.jl <#99 (comment)>:
> + !in(student, course.metadata["students"]) &&
+ (length(prereqIds) == 0 || sum(studentProgress[student.id, prereqIds]) == length(prereqIds)) && # No Prereqs or the student has completed them
+ studentProgress[student.id, course.metadata["id"]] == 0.0 && # The student has not already completed the course
+ !isStudentEnrolled(student, course) && # The student has not already enrolled the course
+ student.termcredits + course.credit_hours <= max_credits && # The student will not exceed the maximum number of credit hours
+ course.metadata["termReq"] <= term && # The student must wait until the term req has been met
+ enrolledInCoreqs(student, course, courses, studentProgress) # The student is enrolled in or has completed coreqs
Done. 3f2a7dc <3f2a7dc>
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I53ARQC7ZFJOXMF2BILR75ONTANCNFSM4PYWCGLA>.
|
|
Greg, I created a new branch called |
|
@jiachengzhang1, is it possible for Greg to just push to master? Then those changes will automatically be included in this pull request. |
|
@haydenfree From what I'm understanding, @heileman made changes on the old version(before yesterday’s commit), I don't know if there any conflict between his edits and new changes. If there isn't, he can just pull, then push his changes to the master. |
|
I did download early yesterday, so we probably need to merge. Do I have access right to push to your repo?
… On Aug 10, 2020, at 8:04 AM, Jiacheng Zhang ***@***.***> wrote:
Greg,
I created a new branch called simulation-dev on jiachengzhang1/CurricularAnalytics.jl, you could push the change to that branch, and I will create a pull request to merge the changes to the master branch.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I57IMATSUZQ5LJLBBUDR7746LANCNFSM4PYWCGLA>.
|
|
@heileman Just sent you an invitation |
|
@heileman I don't think you did a pull and merge before pushing. I think we can either revert or one of you could pull in Jiacheng's last commit on top? Alternatively, Greg, if I reset the repository to be at Jiacheng's previous commit can you add back in the changes you made? You should be able to find them in the commit above and paste them, I'm just not sure what files you meant to change. |
This reverts commit f63a2a9.
|
Just did a git pull, and lost the local changes I made ...
… On Aug 11, 2020, at 1:09 PM, Hayden Free ***@***.***> wrote:
@heileman <https://github.com/heileman> I don't think you did a pull and merge before pushing. I think we can either revert or one of you could pull in Jiacheng's last commit on top?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I56XGIPDRMNMZTS5LMTSAGJPFANCNFSM4PYWCGLA>.
|
|
Jiacheng did a revert since your previous push undid his commit (it had a lot of changes in it). Everything you pushed before is still here: f63a2a9?file-filters%5B%5D=.jl I'm not sure which files you actually implemented changes in vs his changes. Can you take a look and see if you can pick them out? You can re-add them and push |
|
I merged Greg’s commit to simulation-dev branch, and reset the master
branch.
…On Tue, Aug 11, 2020 at 1:39 PM Hayden Free ***@***.***> wrote:
*External Email*
Jiacheng did a revert since your previous push undid his commit (it had a
lot of changes in it). Everything you pushed before is still here: f63a2a9
?file-filters%5B%5D=.jl
<f63a2a9?file-filters%5B%5D=.jl>
I'm not sure which files you actually implemented changes in vs his
changes. Can you take a look and see if you can pick them out? You can
re-add them and push
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#99 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG64P4FUSYJNET55VETINM3SAGUAZANCNFSM4PYWCGLA>
.
|
|
Greg's change is here f63a2a9 |
|
Jiacheng, I made the updates and pushed them to the master branch at your repo.
… On Aug 11, 2020, at 2:39 PM, Hayden Free ***@***.***> wrote:
Jiacheng did a revert since your previous push undid his commit (it had a lot of changes in it). Everything you pushed before is still here: f63a2a9?file-filters%5B%5D=.jl <f63a2a9?file-filters%5B%5D=.jl>
I'm not sure which files you actually implemented changes in vs his changes. Can you take a look and see if you can pick them out? You can re-add them and push
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I5ZAMB3266WAOU4GYADSAGUAZANCNFSM4PYWCGLA>.
|
|
Looks good to me. Should I go ahead and merge to development of main repo? |
|
Sure, I think there we still need to add some documentation. I’ll work on that, but fine to add it after the merge.
… On Aug 12, 2020, at 12:56 PM, Hayden Free ***@***.***> wrote:
Looks good to me. Should I go ahead and merge to development of main repo?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I55GY7WKJN646Y4MOYTSALQV3ANCNFSM4PYWCGLA>.
|
|
Yeah, I'm working some diagrams, and refactoring some of the code which may
be helpful to integrating different models we may use in the future. I can
add them after the merge too.
On Wed, Aug 12, 2020 at 11:58 AM Greg Heileman <notifications@github.com>
wrote:
… *External Email*
Sure, I think there we still need to add some documentation. I’ll work on
that, but fine to add it after the merge.
> On Aug 12, 2020, at 12:56 PM, Hayden Free ***@***.***>
wrote:
>
>
> Looks good to me. Should I go ahead and merge to development of main
repo?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <
#99 (comment)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AAB5I55GY7WKJN646Y4MOYTSALQV3ANCNFSM4PYWCGLA
>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#99 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG64P4HE5ZOR6QOBQI74VT3SALQ3VANCNFSM4PYWCGLA>
.
|
|
Hayden, can you do this now? I want to build a demo notebook using the simulation code.
… On Aug 12, 2020, at 1:03 PM, Jiacheng Zhang ***@***.***> wrote:
Yeah, I'm working some diagrams, and refactoring some of the code which may
be helpful to integrating different models we may use in the future. I can
add them after the merge too.
On Wed, Aug 12, 2020 at 11:58 AM Greg Heileman ***@***.***>
wrote:
> *External Email*
>
> Sure, I think there we still need to add some documentation. I’ll work on
> that, but fine to add it after the merge.
>
> > On Aug 12, 2020, at 12:56 PM, Hayden Free ***@***.***>
> wrote:
> >
> >
> > Looks good to me. Should I go ahead and merge to development of main
> repo?
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub <
> #99 (comment)>,
> or unsubscribe <
> https://github.com/notifications/unsubscribe-auth/AAB5I55GY7WKJN646Y4MOYTSALQV3ANCNFSM4PYWCGLA
> >.
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#99 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AG64P4HE5ZOR6QOBQI74VT3SALQ3VANCNFSM4PYWCGLA>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#99 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I562C2ZSNVQQUGYIHSTSALRRLANCNFSM4PYWCGLA>.
|
This request merges @jiachengzhang1 simulation code. Some clean up is necessary before merging to development, but this request provides a thread to review and discuss changes. I’ll have some more notes and likely some commits by Monday.