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

Added CPP API documentation #236

Merged
merged 8 commits into from
Oct 8, 2020
Merged

Added CPP API documentation #236

merged 8 commits into from
Oct 8, 2020

Conversation

riclarsson
Copy link
Contributor

You were a bit too quick with the merge Oliver, I thought I would have time to add some documentation before the merge.

This adds documentation, renames some types, and allow setting some more global states.

I would like someone to comment on if the documentation makes sense.

I am unsure about the rename and would be willing to revert it. Before ARTS::Var::WorkspaceIndex was a thing. To be more consistent, it is now called ARTS::Var::Index. The latter does not interfere with the global Index, and everything inside the Var namespace knows it has to use a Var::Index. Everywhere else, it is anyways necessary to use Var::Index, so this does not interfere either with Var::Index. It thus made more sense to me to have as common a tongue as we can have here. The hope is anyways that when modules are a thing that there will be no access to the global Index, so then the namespaces having their own similarly named types makes a lot more sense than to have the longer WorkspaceIndex abomination of a name. Of course, this all applies to all Groups.

@olemke
Copy link
Member

olemke commented Oct 6, 2020

No problem. We should make more use of the in progress label for PRs to indicate that they're not ready yet to avoid that.
ARTS::Var::Index sound good to me and more intuitive than WorkspaceIndex.
I'll have a read through the documentation.

doc/uguide/cpp_api.tex Outdated Show resolved Hide resolved
Copy link
Member

@olemke olemke left a comment

Choose a reason for hiding this comment

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

Documentation looks good to me. Just marked some typos. Thanks for providing documentation for the interface.

doc/uguide/cpp_api.tex Outdated Show resolved Hide resolved
doc/uguide/cpp_api.tex Outdated Show resolved Hide resolved
doc/uguide/cpp_api.tex Outdated Show resolved Hide resolved
doc/uguide/cpp_api.tex Outdated Show resolved Hide resolved
Richard Larsson and others added 2 commits October 7, 2020 11:11
We currently still support building without OpenMP. This adds an ifdef
to fix a compile error for that scenario.
@olemke
Copy link
Member

olemke commented Oct 8, 2020

From my side this PR is ready for master after you merge riclarsson#8 to fix the non-OpenMP build issue. Anything else pending from your side?

@riclarsson
Copy link
Contributor Author

riclarsson commented Oct 8, 2020

There is plenty of small things that could be done to improve this but there are no show-stoppers. Some are modular and some are not. A short list:

  • Add namespace ARTS::Constant { using namespace ::Constant; } to autoarts.h to ensure that external users always can use the same exact constants.
  • Add ARTS::Group::Internal for things like MRecord and Absorption::SingleLine to have a life within the ARTS namespace. This one is very manual but would allow setting up most of your calculations quite easily from within the ARTS namespace.
  • Add all the methods without any defaults and with full control over in-and-out parameters to ARTS::Method taking a true Group type and to ARTS::AgendaMethod taking a Var type. This is easy since Verbosity is never input normally, so the signature would be unique. (But verbosity is ignored by AgendaMethod, so it is ugly.)
  • Add all the methods with partial defaults to ARTS::Method taking Group type and to ARTS::AgendaMethod taking Var type. This one is more tricky since the signature might get weird overlaps with the GIN/GOUT only functions. A variadic function would have to be setup and the minimum number of arguments it has to take is to cover all GIN without defaults. The rest is possible with structs with default arguments but I can imagine it getting complicated.
  • Make the signatures work with named inputs. This is very tricky and I don't think it is possible without a lot of ugliness or meta-programming tricks. Perhaps the same struct trick can be used again but using the C-default argument style of C99/C++20. This means the functions have to be called using some very ugly tricks like VectorNLogSpace(ws, {.out=Var::f_grid(ws), .nelem=2, .start=1e5, .end=1e-2});...

If any of these things are "easy" and "directly useful now", I would have added them. Presently, there is no modules so they are not pressing, and the variadic stuff is just too complicated.

Edit: So so long as the failing test isn't really failing but yet another github artifact, I am fine with merging now.

@olemke olemke merged commit 867c9d7 into atmtools:master Oct 8, 2020
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.

2 participants