Skip to content

warnings in math dependencies #727

Closed
Closed
warnings in math dependencies#727
Milestone
2.18.0

Description

@syclik
syclik
opened on Jan 20, 2018

Summary:

The current develop branch has warnings when running:

make test-math-dependencies

Description:

We shouldn't have any warnings. (A lot of these go away if we ever merge scal, arr, mat, but while we still have that distinction, we should maintain the separation.)

It looks like Jenkins doesn't check for this on pull requests. @seantalts, mind looking into that?

Reproducible Steps:

> make test-math-dependencies

Current Output:

> make test-math-dependencies
stan/math/prim/scal/prob/exp_mod_normal_rng.hpp(21): File uses std::vector. [scal]
stan/math/prim/scal/prob/skew_normal_rng.hpp(19): File uses std::vector. [scal]
stan/math/prim/scal/prob/cauchy_rng.hpp(19): File uses std::vector. [scal]
stan/math/prim/scal/prob/student_t_rng.hpp(19): File uses std::vector. [scal]
stan/math/prim/scal/prob/double_exponential_rng.hpp(20): File uses std::vector. [scal]
stan/math/prim/scal/prob/gumbel_rng.hpp(19): File uses std::vector. [scal]
stan/math/prim/scal/prob/logistic_rng.hpp(19): File uses std::vector. [scal]
stan/math/prim/scal/prob/normal_rng.hpp(19): File uses std::vector. [scal]
stan/math/prim/scal/prob/exp_mod_normal_rng.hpp(22): File uses Eigen. [scal]
stan/math/prim/scal/prob/skew_normal_rng.hpp(20): File uses Eigen. [scal]
stan/math/prim/scal/prob/cauchy_rng.hpp(19): File uses Eigen. [scal]
stan/math/prim/scal/prob/cauchy_rng.hpp(20): File uses Eigen. [scal]
stan/math/prim/scal/prob/student_t_rng.hpp(20): File uses Eigen. [scal]
stan/math/prim/scal/prob/double_exponential_rng.hpp(20): File uses Eigen. [scal]
stan/math/prim/scal/prob/double_exponential_rng.hpp(21): File uses Eigen. [scal]
stan/math/prim/scal/prob/gumbel_rng.hpp(19): File uses Eigen. [scal]
stan/math/prim/scal/prob/gumbel_rng.hpp(20): File uses Eigen. [scal]
stan/math/prim/scal/prob/logistic_rng.hpp(19): File uses Eigen. [scal]
stan/math/prim/scal/prob/logistic_rng.hpp(20): File uses Eigen. [scal]
stan/math/prim/scal/prob/normal_rng.hpp(19): File uses Eigen. [scal]
stan/math/prim/scal/prob/normal_rng.hpp(20): File uses Eigen. [scal]

Expected Output:

No warnings.

Additional Information:

Looks like it got in with a pull request for vectorized rngs?

Current Version:

v2.17.0

Activity

  1. mcol commented on Jan 22, 2018

    @mcol
    mcol
    on Jan 22, 2018
    Member

    Commit 8e83e4f introduced them: they appear in the documentation rather than in the code, but the regular expression used to spot them doesn't know about this.

  2. syclik commented on Jan 23, 2018

    @syclik
    syclik
    on Jan 23, 2018
    MemberAuthor

    Thanks! That really helps. I'll try to fix it.

  3. seantalts commented on Jan 23, 2018

    @seantalts
    seantalts
    on Jan 23, 2018
    Member

    Were you asking me to look into making Jenkins fail if test-dependencies shows warnings? I was just going to squash the directories and remove the test, haha.

  4. syclik commented on Jan 23, 2018

    @syclik
    syclik
    on Jan 23, 2018
    MemberAuthor

    Not sure what you're asking. We should have caught this before it got merged. This one's in the doc, so we should be able to fix it easily.

    Regarding squashing directories, that's good, but it's not something that we should do lightly. This is something we should have a discussion about on discourse and give it some serious thought. It's not something that can be undone easily. It'll affect any users that use the math library directly. And... it'll have a large impact on our meta library. I think all for the better, but this is a big decision. Want to kick off the discussion on discourse? Of course, we'll also need a plan for this. It'll really touch a lot of things at once in the math library.

    And we shouldn't be removing all of these tests, just the ones that check for misuse of std::vector and Eigen.

  5. seantalts commented on Jan 23, 2018

    @seantalts
    seantalts
    on Jan 23, 2018
    Member

    The jobs were set up to let the math dependencies warnings slide by without setting the build to failed. You can see that in the old Math jobs here. I copied that behavior into the new pipeline jobs.

    Reason for squashing and ignoring the tests until then being that we at one point decided that we didn't have a reason to keep them separate anymore and it seemed like you and Bob were fine with the sort of change in moral imperative such that we no longer want to support people who want to use the math library without Eigen, and even without the autodiff stuff (there was some of this conversation here). I added a new thread here to explicitly announce our intentions and see if anyone was using the functionality.

    Are you saying there are other math-dependencies checks we want to fail builds? I haven't looked too closely at them.

  6. syclik
    added a commit that references this issue on Jan 23, 2018

    Fixes #727. Just changing doc so it passes the warnings

    b42a665

  7. syclik commented on Jan 23, 2018

    @syclik
    syclik
    on Jan 23, 2018
    MemberAuthor

    Re: old math jobs. I see the current behavior, but it used to fail when there were any warnings. I didn't see any radical changes to the config, so maybe it's an updated Jenkins / plugin? Either way, we want to stop pull requests when it fails that test (even now).

    Let's continue the discussion on discourse.

  8. seantalts
    closed this as completedin b42a665on Jan 24, 2018
  9. seantalts
    added a commit that references this issue on Jan 24, 2018

    Merge pull request #731 from stan-dev/feature/issue-727-warnings

    7a0a8d8
  10. seantalts
    added this to the 2.18.0 milestone on Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions