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

Conversion of enlisted variable to int64 datatype #1420

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

DhruvSondhi
Copy link
Contributor

@DhruvSondhi DhruvSondhi commented Jan 24, 2021

This PR aims to fix #1389, change of data type of the enlisted variables of enlisted

Description

  • Fixed the data type from float64 to int64

Motivation and Context

Solving #1389

How Has This Been Tested?

  • Testing pipeline
  • Other (please describe)

Checking the changes through local test too 😄

Ran on tardis env created through tardis3_env.yml

Changes the data type of the main variables in Montecarlo simulation runner

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • None of the above (please describe)

Checklist:

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have built the documentation on my fork following these instructions
  • I have assigned and requested two reviewers for this pull request

@codecov
Copy link

codecov bot commented Jan 24, 2021

Codecov Report

Merging #1420 (98baa39) into master (6a8403b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1420   +/-   ##
=======================================
  Coverage   71.16%   71.16%           
=======================================
  Files          67       67           
  Lines        5521     5521           
=======================================
  Hits         3929     3929           
  Misses       1592     1592           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a8403b...98baa39. Read the comment docs.

@andrewfullard
Copy link
Contributor

Thanks for your contribution! Please edit this pull request following the PR template that appears on github when you make a PR. You should read the documentation for running unit tests locally to make sure they pass before making a PR.

@@ -127,16 +127,16 @@ def montecarlo_main_loop(
"""
output_nus = np.empty_like(packet_collection.packets_output_nu)
last_interaction_types = (
np.ones_like(packet_collection.packets_output_nu) * -1
np.ones_like(packet_collection.packets_output_nu).astype(np.int64) * -1
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the dtype=np.int64 keyword argument in the array definition, instead of recasting the array to int64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure understood ... I wanted to know that if I usedtype=np.int64 & if I used astype(np.int64) what is the main difference that may occur in the execution of the loop & hence generation of results? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I don't know! I recommend investigating yourself. I suggest the dtype kwarg because it is part of the array creation rather than acted upon the array, so there may be an order-of-operations performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will check it out & convey it here too 😄 ... went with the astype modification as the variables here so as to follow the same pattern 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

So on that line, there is not an array being created, so they have to be recast to a new data type using astype. When an array is created in numpy, you can specify the dtype at creation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I had read about this 😄 ... Thank You very much for clarifying this ... Makes a lot of sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ready now!! ... please do let me know anything I can help with :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,
I found this & does look like a complete good explanation for the difference

@andrewfullard
Copy link
Contributor

/azp run TARDIS refdata

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@andrewfullard
Copy link
Contributor

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@andrewfullard
Copy link
Contributor

FYI I am running a test suite we have, don't worry about the results

@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Jan 25, 2021

I will try to solve some more issues :) ... Thank you very much @andrewfullard for the awesome insight for the usage of astype & dtype :) ... Happy to contribute :)

Though I do have to share my insights too on the difference between the two :P

@azure-pipelines
Copy link

Build succeeded 9fd1587

Click here to see results.

@andrewfullard andrewfullard merged commit 4a6a8a6 into tardis-sn:master Jan 25, 2021
@DhruvSondhi DhruvSondhi deleted the data-type-correction branch January 25, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect data types for real packet logging output data
2 participants