Skip to content

Conversation

@jayantk
Copy link
Contributor

@jayantk jayantk commented Jan 12, 2022

No description provided.


/* Extract the p25

There are many variants with subtle tradeoffs here. One option is
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about this for a while, I believe this is correct. However, this comment confused me a bit because it was hard to reason through the indexes pointed to by the various floor / ceils. I suggest revising this paragraph to:

"There are many variants of the p25 in the case where the number of samples cnt is divisible by 4. When cnt is not divisible by 4, then floor(cnt / 4) always gives you the p25. When cnt is divisible by 4, the p25 falls between the samples x = (floor(cnt/4) -1) and y = floor(cnt/4)."

Then describe the variants in terms of x and y

/* Optimized handling of base cases */

# include "sort_stable_base.c"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually see SORT_STABLE_CE being used anywhere here (?)

/* If left subsort result ended up in orig array, merge into temp
array. Otherwise, merge into orig array. */

if( yl==xl ) x = t;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused by how this function handles the two arrays. Does it flip back and forth between putting the results in the original array and the temp array? Maybe worth a comment

}
}

# if 0 /* These variants of the above don't seem to produce much better code */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

delete this block since it isn't used (?)

Note: price_model_cnt_valid( cnt ) implies
int64_sort_ascending_cnt_valid( cnt ) currently.

Note: consider filtering out "NaN" quotes (i.e. INT64_MIN)? */
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what situation we might get "NaN" quote?

@Reisen Reisen mentioned this pull request Feb 21, 2022
@Reisen Reisen self-requested a review February 23, 2022 17:39
@jayantk
Copy link
Contributor Author

jayantk commented Apr 15, 2022

superseded by #145

@jayantk jayantk closed this Apr 15, 2022
guibescos pushed a commit that referenced this pull request Aug 2, 2022
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