search

CurricularAnalytics / CurricularAnalytics.jl

CurricularAnalytics.jl is a toolbox for studying and analyzing academic program curricula.
GNU Affero General Public License v3.0
23 stars 19 forks source link

Add simulation capabilities #99

Closed haydenfree closed 4 years ago

haydenfree commented 4 years ago

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.

heileman commented 4 years ago

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.

jiachengzhang1 commented 4 years ago

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

haydenfree commented 4 years ago

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.

heileman commented 4 years ago

I agree, the capabilities should be added directly to the toolbox.

heileman commented 4 years ago

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 notifications@github.com wrote:

@jiachengzhang1 commented on this pull request.

In src/Simulation/Enrollment.jl https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#discussion_r467667655:

jiachengzhang1 commented 4 years ago

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.

haydenfree commented 4 years ago

@jiachengzhang1, is it possible for Greg to just push to master? Then those changes will automatically be included in this pull request.

jiachengzhang1 commented 4 years ago

@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.

heileman commented 4 years ago

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 notifications@github.com 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-671374432, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB5I57IMATSUZQ5LJLBBUDR7746LANCNFSM4PYWCGLA.

jiachengzhang1 commented 4 years ago

@heileman Just sent you an invitation

haydenfree commented 4 years ago

@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.

heileman commented 4 years ago

Just did a git pull, and lost the local changes I made ...

On Aug 11, 2020, at 1:09 PM, Hayden Free notifications@github.com 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-672203995, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB5I56XGIPDRMNMZTS5LMTSAGJPFANCNFSM4PYWCGLA.

haydenfree commented 4 years ago

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: https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99/commits/f63a2a9bd46aeb9e4f9e12119cf8486e7cbff870?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

jiachengzhang1 commented 4 years ago

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 notifications@github.com 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/commit/f63a2a9bd46aeb9e4f9e12119cf8486e7cbff870?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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-672267445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG64P4FUSYJNET55VETINM3SAGUAZANCNFSM4PYWCGLA .

jiachengzhang1 commented 4 years ago

Greg's change is here f63a2a9bd46aeb9e4f9e12119cf8486e7cbff870 I'm trying to merge simulation-dev into master, but realize that if merge, the changes made in master will still be overwritten.

heileman commented 4 years ago

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 notifications@github.com 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/commit/f63a2a9bd46aeb9e4f9e12119cf8486e7cbff870?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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-672267445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB5I5ZAMB3266WAOU4GYADSAGUAZANCNFSM4PYWCGLA.

haydenfree commented 4 years ago

Looks good to me. Should I go ahead and merge to development of main repo?

heileman commented 4 years ago

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 notifications@github.com 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-673050992, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB5I55GY7WKJN646Y4MOYTSALQV3ANCNFSM4PYWCGLA.

jiachengzhang1 commented 4 years ago

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 notifications@github.com 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 < https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-673050992>, 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-673051729, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG64P4HE5ZOR6QOBQI74VT3SALQ3VANCNFSM4PYWCGLA .

heileman commented 4 years ago

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 notifications@github.com 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 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 notifications@github.com 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 < https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-673050992>, 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-673051729, 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 https://github.com/CurricularAnalytics/CurricularAnalytics.jl/pull/99#issuecomment-673054526, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB5I562C2ZSNVQQUGYIHSTSALRRLANCNFSM4PYWCGLA.