Skip to content

Conversation

@JonAnCla
Copy link
Contributor

Previously all input data was coerced to float when passed to pymc3.Data()

Attached changes implement this behaviour:

  1. use dtype of passed array, if it has a dtype attr
  2. otherwise test if input data is instance of int
  3. otherwise assume float

WIP for #3813

@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #3816 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3816      +/-   ##
==========================================
- Coverage   90.78%   90.76%   -0.02%     
==========================================
  Files         133      133              
  Lines       20560    20570      +10     
==========================================
+ Hits        18665    18671       +6     
- Misses       1895     1899       +4     

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

Thanks @hottwaj -- looks like a great start!
Just a thought a first glance: I tend to think that np.int8 and np.int16 should be accepted (or at least cast to np.int32). Otherwise, in the current implementation these will raise a ValueError('Unsupported type'), while in fact they are types we want to support. Makes sense?
And I'm sure @rpgoldman will have valuable feedback!

@rpgoldman
Copy link
Contributor

rpgoldman commented Feb 26, 2020

We should add tests to tests/test_data_container.py and not just check that the data creation works properly, but also verify that we can use set_data(), as well.

@JonAnCla
Copy link
Contributor Author

Thanks guys, will make those changes when I have time over the next few days.

@AlexAndorra
Copy link
Contributor

Hey @hottwaj and @rpgoldman ! Do you know where we are on this? Not trying to be pushy, just being curious -- 😄

@JonAnCla
Copy link
Contributor Author

JonAnCla commented Mar 12, 2020 via email

@AlexAndorra
Copy link
Contributor

Hi @hottwaj ! Do you think you'll be able to find some time to finish up this PR? Or do you need me to take over?
The 3.9 release is approaching and it'd be nice to ship it with this new feature 😃

@JonAnCla
Copy link
Contributor Author

Hi Alex, sorry for not making any further progress... Between work and childcare things have been difficult since the UK went into lockdown! I am still a bit too busy to set aside time for this in the short term, feel free to take over and I hope the little I did was useful... If you end up parking it for a bit longer let me know and I can hopefully revisit in another couple of weeks. Thanks!

@AlexAndorra
Copy link
Contributor

I completely understand @hottwaj, the last couple of weeks have indeed been difficult. I'll take this PR over then, and try to push it through for 3.9 😉
Thanks for the work you did, it's definitely helpful! Good luck for the coming weeks and stay safe 😷

@JonAnCla
Copy link
Contributor Author

Great thanks, and hopefully I said this before but thanks for your work on this awesome tool!

@AlexAndorra
Copy link
Contributor

Closing in favor of #3925

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.

3 participants