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

Feature/obbject to dict #5557

Closed
wants to merge 2 commits into from

Conversation

DidierRLopes
Copy link
Collaborator

@jmaslek this should fix the issue with the to_dict() which seemed that it was hitting a regression?

Before
image

After
image

@DidierRLopes DidierRLopes added the feat S Small T-Shirt size Feature label Oct 13, 2023
@DidierRLopes DidierRLopes requested a review from jmaslek October 13, 2023 06:11
@reviewpad reviewpad bot added feat XS Extra small feature and removed feat S Small T-Shirt size Feature labels Oct 13, 2023

return item

return nested_model_to_dict(self.results)
Copy link
Contributor

@montezdesousa montezdesousa Oct 13, 2023

Choose a reason for hiding this comment

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

Does it cover your use case if we just implement in one line?

def to_dict(self) -> Dict
    "Docstring"
    return self.model_dump()["results"]

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦🏻‍♂️Lmao, this what happens when doing a @DidierRLopes task at 1am.

Yeah this for sure the best way to go!!

@deeleeramone
Copy link
Contributor

deeleeramone commented Oct 13, 2023

@jmaslek this should fix the issue with the to_dict() which seemed that it was hitting a regression?

I think this was a choice because as a dict like this, it is actually more of a list and the "fields" are not keyable, you need to convert from a List, whereas like a Dict of Lists, you can key and entire field, like df.to_dict()["close"]

A dict("records") gives you essentially the same thing as df.results. Can we have to_dict() parameterized so that the user define as a preference?

@DidierRLopes
Copy link
Collaborator Author

@jmaslek this should fix the issue with the to_dict() which seemed that it was hitting a regression?

I think this was a choice because as a dict like this, it is actually more of a list and the "fields" are not keyable, you need to convert from a List, whereas like a Dict of Lists, you can key and entire field, like df.to_dict()["close"]

A dict("records") gives you essentially the same thing as df.results. Can we have to_dict() parameterized so that the user define as a preference?

We've had this discussion before in the past, and this was the final decision. List of dicts it's what is used in practice by everyone. Dicts of list is unnatural, so it's fine for us to support it but as to_dict we should have the default one.

And we don't want to leave this as user criteria for the same method since we want to have as much deterministic as possible .

@deeleeramone
Copy link
Contributor

We've had this discussion before in the past, and this was the final decision. List of dicts it's what is used in practice by everyone. Dicts of list is unnatural, so it's fine for us to support it but as to_dict we should have the default one.

And we don't want to leave this as user criteria for the same method since we want to have as much deterministic as possible .

The only reason I suggest it was a choice somewhere, instead of a regression, is that it has been added to the documentation under /core/README.md:

Screenshot 2023-10-13 at 6 53 25 PM

@DidierRLopes
Copy link
Collaborator Author

It was several months ago, but then we changed that decision. Maybe it never got updated.

CC: @jmaslek @piiq

@jmaslek
Copy link
Collaborator

jmaslek commented Oct 14, 2023

This is coming back to me. Darren is correct that this was the reasoning and is indeed not a regression.

The fiasco with List[Dict] vs Dict[List], was with what was being served via api. If you look at obbject.results, it is List[Models], which gets converted into a nice json format.

Screenshot 2023-10-14 at 7 18 32 AM

Dicts of list is unnatural, so it's fine for us to support it but as to_dict we should have the default one.

This is actually misleading. The default to_dict() method (via pandas) is actually returning Dict[Dict], so neither case is default. When I see a to_dict, I assume I am getting back a dictionary of values, which is what we have, rather than a nice typical records format, which is what the api does.

We can add as many different methods to return the data as we want, lets just make sure we document them and clearly explain what they are/how they work.

@DidierRLopes
Copy link
Collaborator Author

Gotcha. Sorry @deeleeramone.

@jmaslek , I'm not getting that output with obb.stocks.load("AAPL").results:

Screenshot 2023-10-14 at 9 34 08 AM

Can we add an argument orient to to_dict, so a user can also do .to_dict(orient="results")?

Screenshot 2023-10-14 at 9 36 17 AM

@jmaslek
Copy link
Collaborator

jmaslek commented Oct 14, 2023

Gotcha. Sorry @deeleeramone.

@jmaslek , I'm not getting that output with obb.stocks.load("AAPL").results:

Screenshot 2023-10-14 at 9 34 08 AM

Can we add an argument orient to to_dict, so a user can also do .to_dict(orient="results")?

Screenshot 2023-10-14 at 9 36 17 AM

Yeah the results output you see is python/pydantic stuff, but on the fastapi end it's converted to what I showed.

Yup we can either add an orient or just skip straight to a to_records/to_json method. Dealers choice.

@DidierRLopes
Copy link
Collaborator Author

Gotcha. Sorry @deeleeramone.
@jmaslek , I'm not getting that output with obb.stocks.load("AAPL").results:
Screenshot 2023-10-14 at 9 34 08 AM
Can we add an argument orient to to_dict, so a user can also do .to_dict(orient="results")?
Screenshot 2023-10-14 at 9 36 17 AM

Yeah the results output you see is python/pydantic stuff, but on the fastapi end it's converted to what I showed.

Yup we can either add an orient or just skip straight to a to_records/to_json method. Dealers choice.

So I suggest doing both:

  • to_dict having the orient='results' as an option
  • to_json returning a JSON with list of dicts

@andrewkenreich @jose-donato any comment on this?

@jmaslek
Copy link
Collaborator

jmaslek commented Oct 30, 2023

Closing in favor of #5588

@jmaslek jmaslek closed this Oct 30, 2023
@piiq piiq deleted the feature/obbject-to_dict branch November 7, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat XS Extra small feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants