Skip to content

Add Stroop Model to synthetic models#13

Open
TheLemonPig wants to merge 35 commits into
mainfrom
feat/add-stroop-model
Open

Add Stroop Model to synthetic models#13
TheLemonPig wants to merge 35 commits into
mainfrom
feat/add-stroop-model

Conversation

@TheLemonPig

Copy link
Copy Markdown
Contributor

Added stroop_model.py to synthetic models

@TheLemonPig TheLemonPig requested review from benwandrew, hollandjg, musslick and younesStrittmatter and removed request for hollandjg August 18, 2023 18:26
@TheLemonPig

Copy link
Copy Markdown
Contributor Author

Where do we add package dependencies? In this case, the error is caused by not recognizing torch

@younesStrittmatter

Copy link
Copy Markdown

package requirements are added to the dependency section in the pyproject.toml file

@hollandjg

Copy link
Copy Markdown
Member

Is there a strong reason to use PyTorch for this? If we're just doing matrix transformations, numpy should be fine, and is already part of the dependencies. Torch is great for complicated neural network things, but if it's possible to simplify this so it works with numpy alone, that would be better. (PyTorch is a big dependency which we've had trouble with in the past, so it's best to avoid it if possible.)

@TheLemonPig TheLemonPig left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update to match Younes' PR changes

Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated

@benwandrew benwandrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks good, including updates to match Younes' changes, but i don't know about @hollandjg's comment re: PyTorch... ?

@TheLemonPig

Copy link
Copy Markdown
Contributor Author

Is there a strong reason to use PyTorch for this? If we're just doing matrix transformations, numpy should be fine, and is already part of the dependencies. Torch is great for complicated neural network things, but if it's possible to simplify this so it works with numpy alone, that would be better. (PyTorch is a big dependency which we've had trouble with in the past, so it's best to avoid it if possible.)

This is a good point. This would mean changing work done by @musslick 's code, which feels outside of my domain without direction.

Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py

@TheLemonPig TheLemonPig left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Attempting to replace torch with numpy, with help of chatGPT

Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
@benwandrew

Copy link
Copy Markdown
Contributor

Attempting to replace torch with numpy, with help of chatGPT

how'd it go!? superficially looks good, but curious about any testing you performed (or have planned).

@TheLemonPig

Copy link
Copy Markdown
Contributor Author

I haven't done any testing yet. I was under the assumption the prebuilt tests checked the functionality of all models. I am actually quite surprised that all the test passed on the first attempt XD. The only additional thing I can think to test is whether the new model replicates the old results. Unfortunately, I have a poor understanding of how this and the previous model worked and what its results were. I am not sure how I would proceed with that

Comment thread tests/test_bundled_models.py

@younesStrittmatter younesStrittmatter left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please wait for #10 and then change accordingly

Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
Comment thread src/autora/experiment_runner/synthetic/psychology/stroop_model.py Outdated
@benwandrew

Copy link
Copy Markdown
Contributor

Please wait for #10 and then change accordingly

@TheLemonPig i think we will also need to reflect this change in the synthetic models you already merged (yesterday).

musslick and others added 3 commits September 3, 2023 06:27
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>

@musslick musslick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! Might just want to check if it still runs with Younes' suggested changes.

TheLemonPig and others added 9 commits September 5, 2023 14:53
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>
Co-authored-by: Younes Strittmatter <younes_strittmatter@brown.edu>

@benwandrew benwandrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good, as long as it runs with recent changes!

@hollandjg hollandjg removed their request for review September 27, 2023 19:44
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.

5 participants