Skip to content

Conversation

juanitorduz
Copy link
Contributor

@juanitorduz juanitorduz commented Jun 22, 2022

This is an attempt to solve #5877

@ricardoV94 I went through pymc/aesaraf.py but I could not figure out by myself where to add your suggestion. I decided to open a PR anyway but I would appreciate some hints on which function this has to be included and tested 🙏

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #5920 (275ee6f) into main (0b4f248) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5920      +/-   ##
==========================================
+ Coverage   89.53%   89.55%   +0.02%     
==========================================
  Files          73       73              
  Lines       13267    13271       +4     
==========================================
+ Hits        11878    11885       +7     
+ Misses       1389     1386       -3     
Impacted Files Coverage Δ
pymc/aesaraf.py 92.19% <100.00%> (+0.11%) ⬆️
pymc/tuning/starting.py 92.43% <0.00%> (-0.13%) ⬇️
pymc/distributions/continuous.py 97.98% <0.00%> (ø)
pymc/distributions/timeseries.py 78.64% <0.00%> (ø)
pymc/parallel_sampling.py 86.79% <0.00%> (+0.99%) ⬆️

@michaelosthege
Copy link
Member

michaelosthege commented Jun 22, 2022

ArviZ has a dependency on pandas already, so you can import it right away:

https://github.com/arviz-devs/arviz/blob/main/requirements.txt#L6

I'd still add it to the requirements and conda envs though.

A good placement for this function might be close to the top of the file, because there's already a function for converting observed data, which is slightly similar.

Any idea what to do with multi-indexed DataFrames?

@ricardoV94
Copy link
Member

ricardoV94 commented Jun 22, 2022

Oh, we removed pandas dependency on purpose but we didn't check if any of our dependencies required it... That's a shame :/

@juanitorduz
Copy link
Contributor Author

I think this first iteration is ready for review :)

Copy link
Member

@michaelosthege michaelosthege 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 to me, but I'll ask in the chat for a second person to look at it & merge (assuming the pipeline goes green).

@juanitorduz juanitorduz requested a review from ricardoV94 June 30, 2022 08:06
Fix type annotations in tests
@michaelosthege
Copy link
Member

The changes requested by @ricardoV94 were made.
I'll merge now.

Thanks for this contribution @juanitorduz !

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