-
Notifications
You must be signed in to change notification settings - Fork 18
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
Large redesign to add flexibility and user defaults and fewer dependencies #139
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Thanks a lot for the PR. I haven't gone through all this yet, but overall it looks like a very sensible set of improvements.
|
…nt on distribution
I wanted to check how comparable the new backend is compared to what already exists. There are a few settings that might change, but this is to show that the results are very comparable between the two. There is some minor spacing changes when dealing with multicolumn objects, it is otherwise capable of producing very similar tables.
Thanks for the effort, @junder873. I agree with @jmboehm. I think it would be important that the tests are updated (and pass) so that we can see more clearly what changes from user's perspective. (Hopefully, most old code would still run.) We can only merge this if you agree to maintain the new codebase. From a maintainer's perspective, such a big rewrite is really hard to review. Not sure what the perfect solution is here. One option would be to split the PR into small chunks that we can actually review. Another would be to trust the tests and just merge if they look good. |
Thank you both for the input (and the past work on this package). Just to respond to the different comments: I am happy to maintain the package going forward, though I appreciate your input. I am trying to minimize the breaking changes that the user faces, though this isn't perfect (see below). I have focused less on maintaining compatibility on the backend pieces, the changes there are just too big to make that doable. I can try to think through creating multiple pull requests, it is possible that doing the backend first would work, but I would have to think more about how to do it. I have been a little slow to work on the tests because I have been trying to make sure the front end works well and work on the backward compatibility. In the most recent set of changes, I added more backward compatibility and reran the tests to see where things stand. Before I get to those results, a few notes:
RegressionTables.default_fe_suffix(x::RegressionTables.AbstractRenderType) = ""
RegressionTables.default_print_control_indicator(x::RegressionTables.AbstractRenderType) = false
RegressionTables.default_regression_statistics(x::RegressionTables.AbstractRenderType, rrs::Tuple) = [Nobs, R2]
RegressionTables.default_print_estimator(x::RegressionTables.AbstractRenderType, rrs) = true The first two are new features (adding a suffix after fixed effects and printing a yes/no if coefficients are omitted). The next two defaults vary based on conditions, a nonlinear regression will include the Pseudo R2 and the estimator section is only printed if more than one type of estimator is provided. Because these defaults did not exist/are different than the current version of the package, I changed them back to make tests comparable. First, going through the actual table output, for the most part the results are similar. There are a few places where spacing is different, often connected to allowing the Estimator to be more than OLS, IV or NL. The HTML tables are also quite different since the padding information is moved into the style section of the table instead of between each cell. From a user perspective, things are mostly similar. The biggest difference that shows up is that
I don't see any other differences between the current package and this proposal, so I wanted to come back to the differences in In order to keep the creation of these new types as simple as possible, I didn't want to include any actual information with the type, so a file does not fit well. One solution (that feels kind of hacky) is to use multiple dispatch to split the arguments, so something like: const asciiOutput = AsciiTable
const latexOutput = LatexTable
const htmlOutput = HtmlTable
(::Type{T})(file::String) where {T<:AbstractRenderType} = T(), file # returns tuple
default_render(x::Nothing) = AsciiTable()
default_render(x::AbstractRenderType) = x
default_render(x::Tuple{<:AbstractRenderType, String}) = x[1]
default_file(rndr::AbstractRenderType, renderSettings) = nothing
default_file(rndr::AbstractRenderType, renderSettings::Tuple{<:AbstractRenderType, String}) = renderSettings[2]
function regtable(
rrs...;
renderSettings = nothing,
rndr::AbstractRenderType = default_render(renderSettings),
file= default_file(rndr, renderSettings),
...
) This would use the old naming system and allow old code to continue to work (based on some simple testing), probably with a deprecation warning. It is obviously a little ugly and possibly create some confusion if somebody provides both a Once again, I appreciate the input and want to minimize the change for users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please remove these two lines? they lead to an error on CI.
Can you also adjust change the minimum julia version in this line
- '1.8' # Replace this with the minimum Julia version that your package supports. E.g. if your package requires Julia 1.5 or higher, change this to '1.5'. |
Hopefully we can then see the state of the tests.
Co-authored-by: Fabian Greimel <[email protected]>
Thanks! You are proposing the following (sets of) changes.
Ideally, you prepare separate PRs, where each PR contains the minimal changes to introduce your proposed change. However I can imagine that some of these changes would depend on the same fundamental backend work. So it might be beneficial to prepare an initial PR with these backend changes first. I think that's what you're contemplating.
|
I am trying to run CI again. But it doesn't - let me close and re-open this PR |
This reverts commit 4ccecdc.
Helps simplify the overall package
Looks great to me, thanks. No more suggestions from my side. |
@jmboehm Just following up, I can't change the setting to allow the documentation to work properly, once that is done though I am happy to merge this request |
Hi @junder873 , apologies for the delay. I've now added the deploy key and the environment variable to the repo. Is there a way to test to see whether it works? |
As far as I can tell, there is no easy way to test the documenter key. However, as long as the GitHub Pages part works, it is possible to manually upload versions of documentation if there is an error with the key. |
Great. Is this ready to be merged? I feel I've been holding this up longer than it should have been held up. |
I will go ahead and merge it, thank you so much for the suggestions and help. |
This is more or less a complete rewrite of the package to create more flexibility, allow the user to easily set defaults, and move toward the use of extensions with Julia 1.9. I did not originally set out to rewrite everything, but as I went for a few additions it became easier to do so. While this is not 100% complete (need a few more details fixed, documentation, make sure it is all tested), I wanted to post it to get some feedback.
In general, this package does 4 main things:
a. There were several things I wanted to do with regression tables that are currently difficult or impossible. The biggest is I wanted to be able to add an extraline that was between two columns, which is useful if you need to add a statistic that compares two values, e.g.:
There is some clunkiness to this solution. If the user does not use the$R^2$ , Adj. $R^2$ , etc.) or manually adding extralines. I went ahead and expanded those available, (e.g., Pseudo $R^2$ ), but it is now possible for the user to define any new statistic and use it, just as if it was built into the package.
:c
align, then the user can pass aDataRow
, which has its own settable alignment for each cell.b. Another feature is adding statistics. Currently, the user is limited to those provided (
c. A long standing todo in this package is to enable custom block ordering (such as stats in front of fixed effects), this is now possible.
d. The underlying type in this update is a vector of vectors. This means that if the user needs to combine tables in a somewhat unusual way to fit their needs, it should be more possible (I want to do more with this)
a. As an example of this, in my Latex tables, I almost always want to use
tabular*
, but to do so I need to define a newRenderSetting
that would match, which takes ~53 lines of code, even though only 2 lines really need to change. Now, the user would only need those 2 lines to change (the package also now exportsLatexTableStar
which does this as well):b. As another example, I prefer using T-Stats in my tables. In the current package, I would need to set this in every table. Now, this default is settable by the user:
With these changes, I also added a lot of other changes that I think are useful:
order
argument, which keeps all of the coefficients but changes the order (adrop
argument is a work in progress, I think it will be pretty simple)order
anddrop
, these arguments are now more flexible. In the current package, you need to provide a full string of the coefficients you want to keep. This proposed update has 4 options: string, integers, ranges, and regex. Integers and ranges are pretty straightforward, regex applies theoccursin
function, so any coefficient names that match the regex will be used (kept, dropped, higher order).Newspaper Advertising $\times$ Cigarette Price
if usingLatexTable()
. This prevents the "&" symbols from being a problem in Latex Tables, but is also settable by the user if the user prefers\&
or something similar.I am sure there is something in there I forgot. I would very much appreciate any feedback. Where possible, I tried to stick with the current user interface, but obviously with such large changes the interface changes as well. This proposal is also not completely finished, particularly the extensions.
resolves #130, resolves #109, resolves #105, resolves #90, resolves #52, resolves #17, resolves #12
(It would also sort of solve #129 and #128)