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

Update load-sea-constants-modules.R #5

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Update load-sea-constants-modules.R #5

merged 1 commit into from
Apr 30, 2019

Conversation

e-leib
Copy link
Contributor

@e-leib e-leib commented Apr 28, 2019

For FRAC_3 module, one of the features of the problems was named (and therefore coded) incorrectly, though the logic was correct. Problems can be congruent or incongruent. A problem is congruent when the larger fraction also has the larger components (e.g., 3/7 > 2/6). A problem is incongruent when the larger fraction has smaller components (e.g., 3/4 > 4/7). In the old code, this was incorrectly named num_larger.

For FRAC_3 module, one of the features of the problems was named (and therefore coded) incorrectly, though the logic was correct. Problems can be congruent or incongruent. A problem is congruent when the larger fraction also has the larger components (e.g., 3/7 > 2/6). A problem is incongruent when the larger fraction has smaller components (e.g., 3/4 > 4/7). In the old code, this was incorrectly named num_larger.
@monicathieu
Copy link
Collaborator

Thanks for flagging this! It's not the only reference to this column, so another small change is required to finish this patch. module_fractions_lvl_3() in module-modules-sea.R, the one-summary-per-participant function for this module, currently has a hard-coded expectation for the contents of that congruency column (formerly known as num_larger). If you open up the code, it should be pretty clear what to change. If you're able, please update the variable name references there and then this patch should be clear to merge!

@e-leib
Copy link
Contributor Author

e-leib commented Apr 29, 2019 via email

@monicathieu monicathieu merged commit 7030bcb into joaquinanguera:master Apr 30, 2019
@e-leib
Copy link
Contributor Author

e-leib commented May 5, 2019 via email

@monicathieu
Copy link
Collaborator

Hi Elena,

You can make a separate pull request for this (since it's solving a slightly different problem than the last pull request you made). You can insert the code you've written above into the relevant .R document on your personal fork, and then submit the PR, and then I can use GitHub's built-in code review tools to make specific code suggestions if I see anything to revise.

thanks for staying on top of updating the SEA modules! glad to have your expertise on board.

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.

2 participants