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

Add Stroop Model to synthetic models #13

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

TheLemonPig
Copy link
Contributor

Added stroop_model.py to synthetic models

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

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

@younesStrittmatter
Copy link
Contributor

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

@hollandjg
Copy link
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.)

Copy link
Contributor Author

@TheLemonPig TheLemonPig left a comment

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

Copy link
Contributor

@benwandrew benwandrew left a comment

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

Copy link
Contributor Author

@TheLemonPig TheLemonPig left a comment

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

@benwandrew
Copy link
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
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

Copy link
Contributor

@younesStrittmatter younesStrittmatter left a comment

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

@benwandrew
Copy link
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).

Copy link
Contributor

@musslick musslick left a comment

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.

Copy link
Contributor

@benwandrew benwandrew left a comment

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