Skip to content

[SC 7521] Move capital markets notebooks from code sharing to code samples #243

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

Conversation

AnilSorathiya
Copy link
Contributor

@AnilSorathiya AnilSorathiya commented Nov 23, 2024

Internal Notes for Reviewers

  • Move capital markets notebooks from code sharing to code samples
  • Serialize the user defined class __str__ method. If not define then class name
  • Remove the Input and the Param tagging in the output of comparison tests

External Release Notes

  • This notebook demonstrate the support capital markets models in ValidMind

@AnilSorathiya AnilSorathiya added documentation Improvements or additions to documentation highlight Feature to be curated in the release notes internal Not to be externalized in the release notes and removed documentation Improvements or additions to documentation labels Nov 23, 2024
@AnilSorathiya
Copy link
Contributor Author

@johnwalz97 I have removed tagging of Input and Param from the comparison.py. The run_test output generate table with column name "Input: <column_name>" or "Param: <column_name>".

                metadata[f"Input: `{input_name}`"] = _get_input_key(input_obj)

with

                metadata[input_name] = _get_input_key(input_obj)

commit 750ab89
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Fri Nov 29 13:45:57 2024 +0000

    remove printed outputs

commit 48ec315
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Fri Nov 29 13:36:32 2024 +0000

    remove tagging of Param and Input

commit 2b9acd5
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Fri Nov 29 13:36:05 2024 +0000

    refactor the function to remove format error

commit feee888
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Fri Nov 29 13:35:27 2024 +0000

    update notebooks

commit 863bf66
Merge: d26604b f0773a0
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Thu Nov 28 16:59:35 2024 +0000

    Merge branch 'main' into anilsorathiya/sc-7521/move-capital-markets-notebooks-from-code

commit d26604b
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Thu Nov 28 16:39:45 2024 +0000

    serialize obj type if obj class has str method implementation

commit 9f83bac
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Sat Nov 23 00:15:56 2024 +0000

    remove comments

commit d0be73d
Author: Anil Sorathiya <anil@validmind.ai>
Date:   Fri Nov 22 23:59:35 2024 +0000

    Move capital markets notebooks from code sharing to code samples
@johnwalz97 johnwalz97 force-pushed the anilsorathiya/sc-7521/move-capital-markets-notebooks-from-code branch from 750ab89 to 842153c Compare November 29, 2024 17:43
Copy link
Contributor

@johnwalz97 johnwalz97 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

github-actions bot commented Dec 2, 2024

PR Summary

This pull request introduces several changes across multiple Jupyter notebooks and Python modules related to option pricing models and JSON encoding. The key changes are as follows:

  1. Notebook Updates:

    • Removed the 'Data Preparation' section from quickstart_option_pricing_models.ipynb.
    • Corrected a typo in the benchmark test description from "Compaparison" to "Comparison".
    • Adjusted the execution count of code cells for consistency.
    • Updated the method of accessing data from result.metric.summary.results[0].data to result.tables[0].data for plotting results.
    • Removed redundant comments and improved code formatting.
    • Deleted two notebooks: OptionPricer.ipynb and option_pricing_models_vm.ipynb, which contained implementations of option pricing models and sensitivity analysis.
  2. Python Module Enhancements:

    • In validmind/tests/comparison.py, simplified metadata key formatting by removing the "Input:" and "Param:" prefixes.
    • In validmind/utils.py, refactored the NumpyEncoder class to improve readability and maintainability by introducing type handler methods for different data types, including datetime, pandas intervals, numpy data types, and QuantLib dates.

These changes aim to improve the clarity, maintainability, and functionality of the codebase, particularly in the context of option pricing models and JSON data handling.

Test Suggestions

  • Run the updated notebooks to ensure that all cells execute without errors and produce the expected outputs.
  • Verify that the removal of the 'Data Preparation' section does not affect the overall flow and results of the notebook.
  • Test the benchmark test function to confirm that the typo correction does not introduce any issues.
  • Check the plotting functions to ensure they correctly access and display data from result.tables[0].data.
  • Ensure that the deletion of OptionPricer.ipynb and option_pricing_models_vm.ipynb does not affect any dependent scripts or documentation.
  • Test the JSON encoding functionality with various data types to confirm that the refactored NumpyEncoder handles all cases correctly.

Copy link
Contributor

@cachafla cachafla left a comment

Choose a reason for hiding this comment

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

Nice 👌

@cachafla
Copy link
Contributor

cachafla commented Dec 4, 2024

Ready to merge?

@AnilSorathiya AnilSorathiya merged commit cc0c1e8 into main Dec 4, 2024
6 checks passed
@johnwalz97 johnwalz97 deleted the anilsorathiya/sc-7521/move-capital-markets-notebooks-from-code branch December 5, 2024 16:51
@cachafla cachafla removed the internal Not to be externalized in the release notes label Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation highlight Feature to be curated in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants