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 quiz tool kit #3

Merged
merged 10 commits into from
Feb 11, 2024
Merged

Add quiz tool kit #3

merged 10 commits into from
Feb 11, 2024

Conversation

Jiaaming
Copy link
Contributor

@Jiaaming Jiaaming commented Feb 7, 2024

Hi. Based on Prof. Samer's requirements, I've added two quiz helper functions (Google Sheets/Gmail integration) to our library. This is my first time modifying the package, so please don't hesitate to point out any issues or non-standard practices.

For more details: https://east-flax-094.notion.site/Quiz-Toolkit-mecsimcalc-ee9f43e25bf8429b9a01cd7095273361

@kwiskel kwiskel requested a review from w3ichen February 8, 2024 18:14
mecsimcalc/__init__.py Outdated Show resolved Hide resolved
PyTests/test_quiz.py Outdated Show resolved Hide resolved
mecsimcalc/quiz_utils.py Outdated Show resolved Hide resolved
Appends values to a Google Sheet, optionally including a timestamp.

Parameters:
service_account_info (dict): The service account credentials.
Copy link
Contributor

Choose a reason for hiding this comment

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

Try using the format used for other functions in the library. It follows the NumPy docstring style guide. Your current style does not display well on MecSimCalc (not sure why)

https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I follow the docstring style guide in this link(which looks like is not exact the same with other function). Let me know if there still have problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks good, however the reason why I repeated the function in the first line is because MecSimCalc doesn't normally display the function. however VS Code does.

Also can you update the other functions to follow the official style guide. I'm not sure why I didn't do it that way originally

here is how it looks on VS Code (shows function + docstring)
image

VS MecSimCalc (Just shows docstring)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jiaaming please follow @PointlessUser 's instructions. He's the original author of this repository and will give final approval for this PR to be merged. Thanks

Copy link
Contributor Author

@Jiaaming Jiaaming Feb 10, 2024

Choose a reason for hiding this comment

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

Yes i can update it for other functions. So do you mean keep the first line in order to be viewed normally on Mecsimcalc?
I'm not sure how you preview docstring in MecSimCalc, sorry. Can you give me a bit instruction on it? So I can check it's format, thank you.


Currently I use Sphinx. My function looks like in HTML:

Screenshot 2024-02-09 at 17 38 54

Other functions like this:
Screenshot 2024-02-09 at 17 39 30

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes keep the first line (the function) so it shows up on mecsimcalc.

To view the docstring on mecsimcalc, you just hover over the function. example:

import mecsimcalc as msc
msc.print_plot()

if you hover your mouse over print_plot() it'll show the docstring

On Sphinx, it should look exactly like yours, except you should add the function at the top of the docstring (like mine)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're still confused, I could do the changes once we accept your pull request

@w3ichen w3ichen requested review from PointlessUser and removed request for PointlessUser and w3ichen February 9, 2024 20:39
@PointlessUser
Copy link
Contributor

Heads up, if you want to publish this, you need to update the documentation. However if you are planning to work on this more, this should be fine

@Jiaaming
Copy link
Contributor Author

Jiaaming commented Feb 10, 2024

Heads up, if you want to publish this, you need to update the documentation. However if you are planning to work on this more, this should be fine

Sure. One more thing: The docs: https://github.com/MecSimCalc/MecSimCalc-docs/blob/main/docs/mecsimcalc-library.md
which shows at https://docs.mecsimcalc.com/mecsimcalc-library need me to update again once we publish the packages, right?

@PointlessUser
Copy link
Contributor

Heads up, if you want to publish this, you need to update the documentation. However if you are planning to work on this more, this should be fine

Sure. One more thing: The docs: https://github.com/MecSimCalc/MecSimCalc-docs/blob/main/docs/mecsimcalc-library.md which shows at https://docs.mecsimcalc.com/mecsimcalc-library need me to update again once we publish the packages, right?

Yes, we have to update both the the readme in this library and the docs in the MecSimCalc-docs.

The most tedious part is updating the source links.
I can help you with that once you get there

Examples
--------
Without metadata:
>>> input_file = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUA..."
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we had input_file = inputs["input_file"] is because this is how users will store the data on mecsimcalc

Although this might be the actual data being stored, but nobody will input it this way
input_file = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUA..."

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this for all other functions as well


Examples
--------
Without Download Link:
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason why the original was in this format:


>>> input_file = inputs["input_file"]
>>> df = msc.input_to_dataframe(input_file)

is because this is how users would normally use the function.

Changing it to:
df = pd.DataFrame({'A': [1, 4], 'B': [2, 5], 'C': [3, 6]})
makes it harder for new users to understand. Especially non experienced programmers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't pay attention to that, sorry. I go through them again and reset examples to original format.

Copy link
Contributor

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

@Jiaaming
Copy link
Contributor Author

Looks good

Thanks. Can we publish it? I will also update the mecsimcalc-docs on the website.

@w3ichen w3ichen merged commit 8c2ef07 into MecSimCalc:main Feb 11, 2024
7 checks passed
@w3ichen
Copy link
Contributor

w3ichen commented Feb 11, 2024

Looks good

Thanks. Can we publish it? I will also update the mecsimcalc-docs on the website.

@Jiaaming approved and merged!

NEXT STEPS:

  • Need to update the docs
  • Need to re-build the default-2022 and default-2023 environment to include this new version. Essentially re-build the environment from any account, making sure that the requirements.txt is same. Then once created, in the Django admin side, change the image field to point to the new aws url. @kwiskel has access to the admin portal

@Jiaaming
Copy link
Contributor Author

Looks good

Thanks. Can we publish it? I will also update the mecsimcalc-docs on the website.

@Jiaaming approved and merged!

NEXT STEPS:

  • Need to update the docs
  • Need to re-build the default-2022 and default-2023 environment to include this new version. Essentially re-build the environment from any account, making sure that the requirements.txt is same. Then once created, in the Django admin side, change the image field to point to the new aws url. @kwiskel has access to the admin portal

Got it, thanks.

@kwiskel
Copy link

kwiskel commented Feb 12, 2024

@Jiaaming don't forget to update the README here when you update the docs

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.

4 participants