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

Enabling github templates for Issues and PRs #1893

Merged
merged 12 commits into from
Aug 10, 2017
Prev Previous commit
Next Next commit
Update ISSUE_TEMPLATE.md
larskotthoff authored Jul 13, 2017
commit ca72fb70c422f2183881ba34f8d61e2b111064e4
10 changes: 5 additions & 5 deletions .github/ISSUE_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
For bug reports please respect the following guidelines and check the boxes accordingly.
For bug reports, please respect the following guidelines and check the boxes accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we prefer a badly written bug reports to no bug report at all? If so, maybe say so here ("these are suggestions, not rules" or something).

Copy link
Member

Choose a reason for hiding this comment

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

In my experience badly written bug reports are mostly useless, so I'd prefer to keep this as it is.


For questions of the type: "How can this be done?": Please consider asking them on stackoverflow.
For questions of the type: "How can this be done?", please consider posting them on Stackoverflow.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know SO so well, but can we make a link to the relevant "tag" or whatever they call it, or possibly tell people to tag their relevant questions 'mlr'?


For everything else ignore this template.

## Bug report

- [ ] Start a new R session
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly ask people to use --vanilla or something? I don't know how it looks in RStudio, but if people use the command line R, they might have some .RData which they overlook.

Copy link
Member Author

Choose a reason for hiding this comment

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

Stuff in the .RData should not compromise mlrs functionality. Loaded namespaces are another thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Willing to bet?

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

library("mlr")
library("parallelMap")
parallelStartMulticore(2)
parallelMap(function(x) resample("classif.IBk", pid.task, cv5), 1:2, simplify=FALSE)

works if run in R --vanilla, but does not work when run in an R session that has an "RWeka" model in its save file. If you do the above after running

library("mlr")
m = train("classif.IBk", pid.task)
q("yes")

it will hang.

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

I guess most things are obviously bugs or non-bugs when run in any kind of environment; requiring the user to start a new R session (or maybe not even that) will be good enough for like 99% of all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't bet 😉 That is a really strange behavior. Do we have an issue for that?

Copy link
Contributor

@mb706 mb706 Jul 17, 2017

Choose a reason for hiding this comment

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

This is mostly described in #1898; afaics there is not much we can do about it. There is also a way of triggering #1379 this way, which is really a PITA if you don't know what's going on.

What I think happens is that objects in .RData that reference namespace environments in some way cause these namespaces to load.

Copy link
Member

Choose a reason for hiding this comment

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

we dont want to overcomplicate this here. people should not be forced to run vanilla.
yes that CAN influence stuff, but the probability as very low. as low as the amount of people who will not be confused by this request.

- [ ] Install the latest version of caret: `update.packages(oldPkgs="mlr", ask=FALSE)`
- [ ] Install the latest version of mlr: `update.packages(oldPkgs="mlr", ask=FALSE)`
- [ ] If you use a GitHub install of mlr: `devtools::install_github(c("BBmisc", "ParamHelpers", "mlr"))
Copy link
Contributor

Choose a reason for hiding this comment

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

closing backtick is missing

Copy link
Contributor

Choose a reason for hiding this comment

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

So how is someone supposed to tick all these boxes? Install the latest cran mlr version and then install it from github again?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will make one sentence

- [ ] [Write a minimal reproducible example](http://stackoverflow.com/a/5963610)
- [ ] [Give a minimal reproducible example](http://stackoverflow.com/a/5963610)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not linking to an explanation what exactly comprises the kind of example we want. If I were just on the fence of reporting a bug, someone expecting me to read all of this this would push me towards not doing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, possibly explicitly state to surround code blocks with ```R. This is actually the one thing I think people should do better in the bug reports I've seen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove the link. People will hopefully know what a reproducible example is.

- [ ] run `sessionInfo()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not just get really cluttered bug reports when we ask for this? When was the last bug where this would have helped us recognize the issue faster?

Copy link
Member

Choose a reason for hiding this comment

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

the sessionInfo helps with problem in eg learners. is see @mb706 point, but IMO it is easy to scroll over the sessionInfo. it dont think it hurts


### Minimal reproducible example:

```r
lrn = makeLearner("classif.lda")
resample(learner = lrn, task = iris.task, resampling = cv10)
```
```